1352056 - call nsIFrame::StyleDisplay less from nsFrame::FinishAndStoreOverflow

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[David Baron :dbaron:]

We call nsIFrame::StyleDisplay an awful lot from nsFrame::FinishAndStoreOverflow, and we could avoid calling it multiple times, given that it shows up in profiles.

Comment 1

[David Baron :dbaron:]

Attachment: Add nsIFrame::Style*WithOptionalParam helpers

MozReview-Commit-ID: DFGBeKtg7hI

Comment 2

[David Baron :dbaron:]

Attachment: Call nsIFrame::StyleDisplay less from nsFrame::FinishAndStoreOverflow

MozReview-Commit-ID: 5zuNLfyPv8o

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8852932 [details] [diff] [review]
Add nsIFrame::Style*WithOptionalParam helpers

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

r=me with one nit:

::: layout/generic/nsIFrame.h
@@ +766,5 @@
>     * Callers outside of libxul should use nsIDOMWindow::GetComputedStyle()
>     * instead of these accessors.
> +   *
> +   * Callers can use Style*WithOptionalParam if they're in a function that
> +   * has an optional parameter to pass the style struct.

Nit: the last line is a bit ambiguous here.  The phrase "to pass the style struct" kind of sounds like the style struct is the *recipient* of something that's being passed.  (That's the way I read this initially, and it didn't make sense to me.)

Maybe reword to avoid that ambiguity?  e.g. maybe something like:
 Callers can use the Style*WithOptionalParam convenience wrapper if they're
 in a function that accepts an optional pointer to this style struct.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8852933 [details] [diff] [review]
Call nsIFrame::StyleDisplay less from nsFrame::FinishAndStoreOverflow

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

r=me. One suggestion:

::: layout/generic/nsFrame.cpp
@@ +8913,4 @@
>    nsRect bounds(nsPoint(0, 0), aNewSize);
>    // Store the passed in overflow area if we are a preserve-3d frame or we have
>    // a transform, and it's not just the frame bounds.
> +  if (Combines3DTransformWithAncestors(disp) || hasTransform) {

Nit: As long as you're making us precompute the "hasTransform" condition here, we probably should swap the order of the conditions -- i.e. this should perhaps be:
 if (hasTransform || Combines3DTransformWithAncestors(disp)) {

That way, we can skip the Combines3DTransformWithAncestors(disp) call, if we already know hasTransform is true.

Comment 5

[David Baron :dbaron:]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f4de47b66b2ab1b4c0dc54d91a94e07668b58f
Bug 1352056 - Add nsIFrame::Style*WithOptionalParam helpers.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/b845e6e9c44778ba6cb60a1cef30ec9be4392fe9
Bug 1352056 - Call nsIFrame::StyleDisplay less from nsFrame::FinishAndStoreOverflow.  r=dholbert

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3f4de47b66b
https://hg.mozilla.org/mozilla-central/rev/b845e6e9c447