841192 - Remove nsDisplayClip(RoundedRect) and attach explicit Clip objects to every display item

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In some display lists (e.g. in bug 840372) a lot of nested nsDisplayClips are created. Constructing these, and processing them in FrameLayerBuilder is expensive and unnecessary. nsDisplayClip also adds a lot of complications to display lists because unlike all other display items, they may not have a specific "underlying frame". Also, FrameLayerBuilder eventually constructs explicit Clip objects for each (post-optimization) display item anyway.

So, assigning explicit Clip objects during display list construction should save a lot of work and simplify the code.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Plan of attack:

1) Move mozilla::FrameLayerBuilder::Clip to mozilla::DisplayItemClip.
2) Instead of wrapping up display items after we've constructed them, we'll keep "current clip" state in nsDisplayListBuilder with an Auto class to save and restore that state. When a display item is created, the current clip in the nsDisplayListBuilder will be copied to a new DisplayItemClip in the displaylist arena and a pointer to that DisplayItemClip will be put in the new display item. We can cache this pointer so that display items allocated with the same current clip state share the same DisplayItemClip object (keeping display list size down).
3) The "current clip" state is a bit complex. It can't just be a DisplayItemClip, because for 'overflow' clipping the container needs to clip the child items that it is the containing block for, and *not* clip the other child items. So we're actually going to need one DisplayItemClip which "clips everything" and a separate stack of (DisplayItemClip, containing block) pairs which clip the display items whose frame has the given containing block. For efficiency we probably should store all three of:
a) DisplayItemClip that clips everything
b) Stack of (DisplayItemClip, containing block) with the conditional clips in effect. When we follow placeholders through BuildDisplayListForChild we'd remove any clips that are no longer containing-block ancestors for the out-of-flow frame. And each clip on the stack would include the clips below it on the stack.
c) A pointer to an arena-allocated DisplayItemClip which is the intersection of a) and the clip at the top of stack b), if one has been allocated.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=36a66bf55feb

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Instead of making nsSubDocumentFrame::Reflow enforce that HTML <frame> elements have no padding, do that in ua.css so that all our code agrees on what the content box is

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Move FrameLayerBuilder::Clip to DisplayItemClip

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Make DisplayItemClip members private and encapsulate them in a real API

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Create DisplayListClipState

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: Add "current DisplayListClipState" to nsDisplayListBuilder

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6: Save and restore DisplayListClipState when we start building display lists for a frame

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 7: Move child->MarkAbsoluteFramesForDisplayList call down

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 8: On encountering out-of-flow frames during display list construction, save their clip rect as well as their dirty rect for later use when we traverse the placeholder

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 9: Add extra APIs to DisplayItemClip to set the current state, intersect with another DisplayItemClip, test intersection with a rect, translate, and provide easy access to a DisplayItemClip objec

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 10: Add extra APIs to DisplayListClipState to constrain the current clip, and an Auto class for safe saving and restoring

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 11: Make nsDisplayOptionEventGrabber be a non-anonymous display item list wrapper

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 12: Move RoundedRectIntersectsRect from nsDisplayList.cpp to nsLayoutUtils

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 13: Rename nsFrame::ApplyOverflowClipping to ShouldApplyOverflowClipping

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 14: Convert all usage of nsDisplayClip(RoundedRect) to use DisplayListClipState/DisplayItemClip

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 722467 [details] [diff] [review]
Part 4: Create DisplayListClipState

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

::: layout/base/DisplayItemClip.h
@@ +57,5 @@
>  
>    // Construct as the intersection of aOther and aClipItem.
>    DisplayItemClip(const DisplayItemClip& aOther, nsDisplayItem* aClipItem);
>  
> +  void IntersectWith(const DisplayItemClip& aOther);

Is this meant to be here? I don't see an implementation of it

::: layout/base/DisplayListClipState.h
@@ +36,5 @@
> +  {
> +    return mClipContentDescendants;
> +  }
> +
> +  void SetClipForContainingBlockDescendants(const DisplayItemClip* aClip)

Should these two not have the 'Get' prefix since they are just getters without side effects?

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 722471 [details] [diff] [review]
Part 6: Save and restore DisplayListClipState when we start building display lists for a frame

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

::: layout/base/nsDisplayList.h
@@ +488,5 @@
>    class AutoBuildingDisplayList {
>    public:
>      AutoBuildingDisplayList(nsDisplayListBuilder* aBuilder, bool aIsRoot)
>        : mBuilder(aBuilder),
> +        mPrevClipState(aBuilder->mClipState),

It's a bit scary to me that we just copy the raw pointers within DisplayListClipState and trust that nothing will delete them. I realise that the arena allocations make this true, but we should document it somewhere.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> ::: layout/base/DisplayListClipState.h
> @@ +36,5 @@
> > +  {
> > +    return mClipContentDescendants;
> > +  }
> > +
> > +  void SetClipForContainingBlockDescendants(const DisplayItemClip* aClip)
> 
> Should these two not have the 'Get' prefix since they are just getters
> without side effects?

"GetFoo()" means the functions can return null, "Foo()" means it cannot.

Comment 20

[Mats Palmgren (inactive)]

Attachment: (frame content for testcase)

Comment 21

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 722458 [details] [diff] [review]
Part 1: Instead of making nsSubDocumentFrame::Reflow enforce that HTML <frame> elements have no padding, do that in ua.css so that all our code agrees on what the content box is

It sounds as if you intended this change to be idempotent?
Can you explain why I now see a left and top padding in Testcase #1
with the Part 1 patch applied?

Fwiw, Trident and Webkit appears to implement padding for <frame>,
I tend to think we should too.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Comment on attachment 722467 [details] [diff] [review]
> Part 4: Create DisplayListClipState
> 
> Review of attachment 722467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/DisplayItemClip.h
> @@ +57,5 @@
> >  
> >    // Construct as the intersection of aOther and aClipItem.
> >    DisplayItemClip(const DisplayItemClip& aOther, nsDisplayItem* aClipItem);
> >  
> > +  void IntersectWith(const DisplayItemClip& aOther);
> 
> Is this meant to be here? I don't see an implementation of it

The implementation's in part 9. Sorry, I'll move it down.

(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> It's a bit scary to me that we just copy the raw pointers within
> DisplayListClipState and trust that nothing will delete them. I realise that
> the arena allocations make this true, but we should document it somewhere.

Actually there is something here which makes me a bit uncomfortable: we require that ClipContainingBlockDescendants/ClipContentDescendants and SetClipForContainingBlockDescendants/SetClipForContentDescendants take parameters that will stay alive (on the stack, typically) until the old clip state is restored.

I can probably fix that unsafeness by requiring all DisplayListClipState modification be done through DisplayListClipState::AutoSaveRestore, which would actually contain the temporary DisplayItemClip that gets used for setting. I'll try that.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mats Palmgren [:mats] from comment #22)
> Fwiw, Trident and Webkit appears to implement padding for <frame>,
> I tend to think we should too.

OK.

Comment 25

[Matt Woodrow (:mattwoodrow)]

> 
> Actually there is something here which makes me a bit uncomfortable: we
> require that ClipContainingBlockDescendants/ClipContentDescendants and
> SetClipForContainingBlockDescendants/SetClipForContentDescendants take
> parameters that will stay alive (on the stack, typically) until the old clip
> state is restored.
> 
> I can probably fix that unsafeness by requiring all DisplayListClipState
> modification be done through DisplayListClipState::AutoSaveRestore, which
> would actually contain the temporary DisplayItemClip that gets used for
> setting. I'll try that.

Sounds good.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 722480 [details] [diff] [review]
Part 14: Convert all usage of nsDisplayClip(RoundedRect) to use DisplayListClipState/DisplayItemClip

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

Looks really good! Might be worth getting someone else to double check the nsSubDocumentFrame etc stuff, I don't know that code all that well.

::: layout/base/FrameLayerBuilder.cpp
@@ +3242,5 @@
> +      tmpClip.RemoveRoundedCorners();
> +      clip = &tmpClip;
> +    }
> +    if (currentClipIsSetInContext != clip->HasClip() ||
> +        (clip->HasClip() && *clip != currentClip)) {

Items with the same clip should compare true with just a pointer comparison, right?

Can we make currentClip a pointer and just do that instead here? I guess we might still need to do both (nsDisplayItem::SetClip allocates new memory), but it should be faster in the common case.

::: layout/base/nsDisplayList.cpp
@@ -974,5 @@
> -                 item->GetType() != nsDisplayItem::TYPE_TRANSFORM ||
> -                 item->GetUnderlyingFrame() != aForItem->GetUnderlyingFrame() ||
> -                 aForItem->ReferenceFrame() != aForItem->GetUnderlyingFrame(),
> -                 "If we have an nsDisplayTransform child (for the same frame),"
> -                 "then we shouldn't be our own reference frame!");

Why is this going?

@@ +1636,5 @@
>    nscoord radii[8];
>    return !aFrame->GetBorderRadii(radii) ||
> +         nsLayoutUtils::RoundedRectIntersectsRect(nsRect(aFrameToReferenceFrame,
> +                                                         aFrame->GetSize()),
> +                                                  radii, aTestRect);

Probably should be part of the previous patch, doubt it matters though.

::: layout/base/nsLayoutDebugger.cpp
@@ +159,5 @@
> +    const DisplayItemClip& clip = i->GetClip();
> +    nsCString clipString;
> +    if (clip.HasClip()) {
> +      clipString = clip.ToString();
> +    }

ToString already handles the !HasClip() case

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1 v2

Webkit supports both border and padding on <frame> elements, and it turns out to be trivial for us to support both, so why not?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> Items with the same clip should compare true with just a pointer comparison,
> right?

Not necessarily.

> Can we make currentClip a pointer and just do that instead here? I guess we
> might still need to do both (nsDisplayItem::SetClip allocates new memory),
> but it should be faster in the common case.

We could use a pointer comparison as a shortcut but in the common (non-roundedrect) case comparing clips is already just comparing the rects and checking that both rounded-rect arrays are empty, so really cheap.

> ::: layout/base/nsDisplayList.cpp
> @@ -974,5 @@
> > -                 item->GetType() != nsDisplayItem::TYPE_TRANSFORM ||
> > -                 item->GetUnderlyingFrame() != aForItem->GetUnderlyingFrame() ||
> > -                 aForItem->ReferenceFrame() != aForItem->GetUnderlyingFrame(),
> > -                 "If we have an nsDisplayTransform child (for the same frame),"
> > -                 "then we shouldn't be our own reference frame!");
> 
> Why is this going?

Because we're passing aForItem just for this assertion, which seems not worthwhile.

> @@ +1636,5 @@
> >    nscoord radii[8];
> >    return !aFrame->GetBorderRadii(radii) ||
> > +         nsLayoutUtils::RoundedRectIntersectsRect(nsRect(aFrameToReferenceFrame,
> > +                                                         aFrame->GetSize()),
> > +                                                  radii, aTestRect);
> 
> Probably should be part of the previous patch, doubt it matters though.

Will fix.

> ::: layout/base/nsLayoutDebugger.cpp
> @@ +159,5 @@
> > +    const DisplayItemClip& clip = i->GetClip();
> > +    nsCString clipString;
> > +    if (clip.HasClip()) {
> > +      clipString = clip.ToString();
> > +    }
> 
> ToString already handles the !HasClip() case

Will fix.

Comment 29

[Mats Palmgren (inactive)]

Comment on attachment 726966 [details] [diff] [review]
Part 1 v2

> +  // XUL <iframe> or <browser>, or HTML <iframe>, <object> or <embed>

or <frame> :-)

Seriously though, it seems a bit odd now to list the potential elements
this frame class deals with here, since they are all handled the same.
If anywhere, the comment at the top of nsSubDocumentFrame.cpp seems more
appropriate, and/or in the header file somewhere.

Anyway, r=mats, as you see fit.

Comment 30

[Mats Palmgren (inactive)]

Attachment: Testcase #2

For the record, I noticed a couple of details where rendering differs
between UAs.  <frameset> background propagates to the viewport in IE10,
not in Chrome (27) or Firefox.  <frameset> margins seems mostly compatible
in IE10/FF (top,left,bottom seems correct, but the right margin is zero
in IE and 20px in FF (size of vertical borders?).  Chrome only has a top
margin.  Not sure if this is worth filing bugs on, unless there actually
is a spec for <frameset> / <frame> rendering somewhere.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mats Palmgren [:mats] from comment #30)
> For the record, I noticed a couple of details where rendering differs
> between UAs.  <frameset> background propagates to the viewport in IE10,
> not in Chrome (27) or Firefox.

I believe we're correct per spec. The <frameset> is not the document's root element, nor is it the document's <body> element, so nothing says its background should propagate.

> <frameset> margins seems mostly compatible
> in IE10/FF (top,left,bottom seems correct, but the right margin is zero
> in IE and 20px in FF (size of vertical borders?).

How margins work on <frameset> is not specified anywhere as far as I know. Frameset layout is specified here: http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#frames-and-framesets but that doesn't handle margins (or CSS padding or borders either).

If we had to deal with this I'd be tempted to just force all margins to 0 for <frame> and <frameset> elements, and force borders and padding to 0 for <frameset>s, using !important rules in html.css. But really I don't care.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 15: improve DisplayListClipState API to make it safer

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 16: Take plugin's own clipping into account when computing the region for its window. Make SortByContentOrder handle cases where display items in the same list come from different documents

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=89222f3741a7

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=6cd6de0f5b7e

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1740631994da
https://hg.mozilla.org/integration/mozilla-inbound/rev/698d23ec564b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7633e8733e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3f4cfe3866
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f4ddf03ffb
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2f8190c7dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9214f38c73e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b16876942c8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/30928f33d63f
https://hg.mozilla.org/integration/mozilla-inbound/rev/072417fd4d8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9583f064a2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/54952cb8f869
https://hg.mozilla.org/integration/mozilla-inbound/rev/00fdcb8176a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7f4e26d4a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3396e302a1e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e958f7224a7

Comment 37

[Gary [:streetwolf52]]

I tried out cset 1e958f7224a7 on m-c inbound which seems to only include this patch and I see an anomaly in my sidebars.  I've attached a screenshot of it.  The previous cset 905671d954c5 worked OK.

Comment 38

[Gary [:streetwolf52]]

Attachment: Sidebar anomaly

Comment 39

[Gary [:streetwolf52]]

about:addons also shows the same sort of thing.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1740631994da
https://hg.mozilla.org/mozilla-central/rev/698d23ec564b
https://hg.mozilla.org/mozilla-central/rev/fb7633e8733e
https://hg.mozilla.org/mozilla-central/rev/ec3f4cfe3866
https://hg.mozilla.org/mozilla-central/rev/d6f4ddf03ffb
https://hg.mozilla.org/mozilla-central/rev/dc2f8190c7dd
https://hg.mozilla.org/mozilla-central/rev/a9214f38c73e
https://hg.mozilla.org/mozilla-central/rev/b16876942c8d
https://hg.mozilla.org/mozilla-central/rev/30928f33d63f
https://hg.mozilla.org/mozilla-central/rev/072417fd4d8e
https://hg.mozilla.org/mozilla-central/rev/c9583f064a2d
https://hg.mozilla.org/mozilla-central/rev/54952cb8f869
https://hg.mozilla.org/mozilla-central/rev/00fdcb8176a7
https://hg.mozilla.org/mozilla-central/rev/5f7f4e26d4a9
https://hg.mozilla.org/mozilla-central/rev/3396e302a1e4
https://hg.mozilla.org/mozilla-central/rev/1e958f7224a7

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't see that on Linux, probably because the theme background isn't different.

Comment 42

[Gary [:streetwolf52]]

Filed bug https://bugzilla.mozilla.org/show_bug.cgi?id=858803