Status

()

defect
REOPENED
8 years ago
4 years ago

People

(Reporter: mats, Unassigned)

Tracking

(Blocks 2 bugs, {perf})

Trunk
mozilla12
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

5.34 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.52 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.37 KB, patch
roc
: review+
Details | Diff | Splinter Review
18.15 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
Follow-up from bug 524925 which fixed outline and transform properties
to use UpdateOverflow.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #107)
> I also suggest filing a followup bug to have a good look at CalcDifference
> implementations and see which reflow hints can be converted to
> UpdateOverflow. I think there's quite a few that can be converted...
Mats, can you do this?

This should be some cheap and easy performance wins...
Assignee: nobody → matspal
Comment on attachment 590378 [details] [diff] [review]
Changes to 'text-shadow' and 'box-shadow' only needs to update the overflow areas and repaint, not reflow.  Changes to border style/color/etc only needs repaint, not SyncFrameView.

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

::: layout/style/nsStyleStruct.cpp
@@ +2794,5 @@
>    if (lhs == rhs)
>      return NS_STYLE_HINT_NONE;
>  
> +  const nsChangeHint kUpdateOverflowAndRepaintHint =
> +    NS_CombineHint(nsChangeHint_UpdateOverflow, nsChangeHint_RepaintFrame);

This can be static as well as const
Attachment #590378 - Flags: review?(roc) → review+
Comment on attachment 590380 [details] [diff] [review]
Changes to '-moz-text-blink' and some 'text-decoration-style' values only need to update the overflow areas and repaint, not reflow.

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

::: layout/style/nsStyleStruct.cpp
@@ +2745,5 @@
> +    return NS_STYLE_HINT_REFLOW;
> +  }
> +    
> +  const nsChangeHint kUpdateOverflowAndRepaintHint =
> +    NS_CombineHint(nsChangeHint_UpdateOverflow, nsChangeHint_RepaintFrame);

can be static
Attachment #590380 - Flags: review?(roc) → review+
nsTextFrame::SetSelectedRange does some reflowing to update overflow areas in case the selection drawing requires overflow area changes. I think probably nsTextFrame::UnionAdditionalOverflow and SetSelectedRange can be simplified now. Maybe that requires another bug.
Comment on attachment 590381 [details] [diff] [review]
Introduce NS_STYLE_HINT_OVERFLOW as a short-hand for nsChangeHint(nsChangeHint_RepaintFrame | nsChangeHint_UpdateOverflow).

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

I actually prefer the overflowAndRepaint names. It's just more clear and doesn't impact the wordiness of the code much. So I'd prefer to just leave it as is (and get rid of more of those shortcuts).

::: layout/base/nsChangeHint.h
@@ +143,5 @@
>                 nsChangeHint_ClearDescendantIntrinsics | \
>                 nsChangeHint_NeedDirtyReflow)
>  #define NS_STYLE_HINT_REFLOW \
>    nsChangeHint(nsChangeHint_RepaintFrame | nsChangeHint_ReflowFrame)
> +#define NS_STYLE_HINT_OVERFLOW \

NS_STYLE_HINT_UPDATE_OVERFLOW I think
Attachment #590381 - Flags: review?(roc) → review-
I haven't done an audit myself, but one more opportunity I know about is nsStyleSVGReset::CalcDifference ... mFilter can update overflow area instead of reflowing, and mClipPath and mMask don't even need to do that (since they don't affect the overflow area).
I'd like a short-hand for this in some form, let's go with
NS_STYLE_HINT_UPDATE_OVERFLOW for now since it follows the current scheme.
We can re-organize / rename the nsChangeHint stuff in a separate bug.

The change is just s/NS_STYLE_HINT_OVERFLOW/NS_STYLE_HINT_UPDATE_OVERFLOW/
Attachment #590381 - Attachment is obsolete: true
Attachment #590606 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> nsTextFrame::SetSelectedRange does some reflowing to update overflow areas
> in case the selection drawing requires overflow area changes. I think
> probably nsTextFrame::UnionAdditionalOverflow and SetSelectedRange can be
> simplified now. Maybe that requires another bug.

Filed bug 720270.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> I haven't done an audit myself, but one more opportunity I know about is
> nsStyleSVGReset::CalcDifference ... mFilter can update overflow area instead
> of reflowing, and mClipPath and mMask don't even need to do that (since they
> don't affect the overflow area).

The SVG effects stuff is really complicated (to me) so I think this deserves
deeper analysis.  Filed bug 720272 for that.  Let's take the current improvements
as a first step.
OK.

nsStyleDisplay::mClip shouldn't need to reflow. I can't see any more cases (other than the SVG cases).
I also moved the ReconstructFrame cases to the start, and return early if hit.

layout/reftests/bugs/389224-1.html failed with this change, not sure why.
I changed it to trigger the style change of the 'load' event instead of
'MozReftestInvalidate', then it works.
Attachment #590922 - Flags: review?(roc)
(In reply to Mats Palmgren [:mats] from comment #17)
> layout/reftests/bugs/389224-1.html failed with this change, not sure why.
> I changed it to trigger the style change of the 'load' event instead of
> 'MozReftestInvalidate', then it works.

That sounds like you're just hiding the bug. Please debug!
You're right, sorry.  It looked like just flaky test at first but it does indeed
show a real bug with the UpdateOverflow code.  Same as bug 720987 if I'm not mistaken.
I'll post the bug fix there and block on that.
Depends on: 720987
Updated to tip since there was a conflicting change to
nsStyleVisibility::CalcDifference
Attachment #590379 - Attachment is obsolete: true
Attachment #591672 - Flags: review+
Identical to the last version but without the change to the test so
if you already did review the code part you can just rubber-stamp this.

The fix in bug 720987 makes the test pass.  The problem was that the
stored overflow areas is the clipped areas and there's no code to
pick up the full unclipped child overflow again in the current code
so the areas remained the same after the style change.

Try results: https://tbpl.mozilla.org/?tree=Try&rev=4f0758b3d061
Attachment #590922 - Attachment is obsolete: true
Attachment #590922 - Flags: review?(roc)
Attachment #591692 - Flags: review?(roc)
Depends on: 722117
Depends on: 723669
These patches will be backed out in bug 724432.  I intend to re-land most of it
with some additional fixes here, except text-shadow which is handled by bug 723669.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 726912
Depends on: 764554
What I think I'd written elsewhere:  if we want to do this, I think we should:
 * rename UpdateOverflow to UpdateAncestorOverflow
 * add a new hint called UpdateSelfOverflow
 * implement the new hint such that we start from accumulating the element's overflow from its descendants and its on characteristics (rather than starting with the change from pre-transform overflow to post-transform overflow on the element)
Also, I'd prefer not to have NS_STYLE_HINT_UPDATE_OVERFLOW for two reasons:
 * we should get rid of NS_STYLE_HINT_* and use nsChangeHint_*
 * I'd rather actually have to have the combination written out so that it's clearer.
A good bit of the discussion about relanding this is in bug 723669.
The prerequisites for doing this are now happening in bug 984226.
Assignee: mats → nobody
Depends on: 1194480
You need to log in before you can comment on or make changes to this bug.