call nsIFrame::StyleDisplay less from nsFrame::FinishAndStoreOverflow

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 months ago
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.
(Assignee)

Comment 1

3 months ago
Created attachment 8852932 [details] [diff] [review]
Add nsIFrame::Style*WithOptionalParam helpers

MozReview-Commit-ID: DFGBeKtg7hI
Attachment #8852932 - Flags: review?(dholbert)
(Assignee)

Comment 2

3 months ago
Created attachment 8852933 [details] [diff] [review]
Call nsIFrame::StyleDisplay less from nsFrame::FinishAndStoreOverflow

MozReview-Commit-ID: 5zuNLfyPv8o
Attachment #8852933 - Flags: review?(dholbert)

Updated

3 months ago
Blocks: 1341750
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.
Attachment #8852932 - Flags: review?(dholbert) → review+
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.
Attachment #8852933 - Flags: review?(dholbert) → review+
(Assignee)

Comment 5

3 months ago
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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3f4de47b66b
https://hg.mozilla.org/mozilla-central/rev/b845e6e9c447
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1351830
You need to log in before you can comment on or make changes to this bug.