Bug 216752 – [LFC Display] Add the beginnings of a CSS display box hierarchy and CSS painter

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

[LFC Display] Add the beginnings of a CSS display box hierarchy and CSS painter

[Simon Fraser (smfr)]

Created attachment 409244 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 409248 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 409249 [details]
Patch

[Sam Weinig]

Comment on attachment 409249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409249&action=review

> Source/WebCore/display/DisplayTreeBuilder.cpp:134
> +        if (auto* cachedImage = downcast<Layout::ReplacedBox>(layoutBox).cachedImage())
> +            imageBox->setImage(cachedImage->image());

In what scenario is there no cachedImage here? It would definitely be easier to reason about if the ImageBox could always have an Image.

> Source/WebCore/display/DisplayTreeBuilder.h:47
> +    TreeBuilder(float pixelSnappingFactor);

Should be explicit.

> Source/WebCore/display/DisplayTreeBuilder.h:56
> +    float m_pixelSnappingFactor { 1 };

Since you always pass in a value, there is no need to give this a default. Its probably better that everyone be explicit.

> Source/WebCore/display/css/DisplayBox.h:43
> +        ContainerBox            = 1 << 0,
> +        ImageBox                = 1 << 1,

Seems like a lot of whitespace here.

> Source/WebCore/display/css/DisplayCSSPainter.cpp:180
> +    for (const auto* child = containerBox.firstChild(); child; child = child->nextSibling()) {

Can we structure this to use c++ iterators so we can make this a for (auto child : containerBox.children()) type situation?

> Source/WebCore/display/css/DisplayCSSPainter.h:58
> +    static void paintBox(const Box&, GraphicsContext&, FloatSize paintOffset, const IntRect& dirtyRect);
> +    static void paintBoxDecorations(const Box&, GraphicsContext&, FloatSize paintOffset);
> +    static void paintBoxContent(const Box&, GraphicsContext&, FloatSize paintOffset);
> +
> +    static void recursiveCollectLayers(const ContainerBox&, Vector<const Box*>& negativeZOrderList, Vector<const Box*>& positiveZOrderList);

Since there is no state at all (let alone, private state), these don't really need to be in the header. Could just be declared and defined in the cpp.

> Source/WebCore/display/css/DisplayImageBox.h:41
> +    WTF_MAKE_FAST_ALLOCATED_WITH_HEAP_IDENTIFIER(InageBox);

Typo. InageBox -> ImageBox

> Source/WebCore/display/css/DisplayImageBox.h:43
> +    ImageBox(FloatRect borderBox, Style&&);

I wonder if we can make this just take a Ref<Image>&&, and not have the setter. Is there ever a time we want an ImageBox without an Image?

> Source/WebCore/display/css/DisplayReplacedBox.h:41
> +    ReplacedBox(FloatRect borderBox, Style&&, OptionSet<Flags>);
> +    
> +    FloatRect replacedContentRect() const { return m_replacedContentRect; }
> +    void setReplacedContentRect(FloatRect);

Like with ImageBox, can we pass in the replacedContentRect to the constructor, and avoid the setter?

> Source/WebCore/display/css/DisplayStyle.h:35
> +#include <wtf/OptionSet.h>
> +

Should probably include <wtf/Optional.h> since you rely on it.

> Source/WebCore/display/css/DisplayStyle.h:52
> +        Positioned          = 1 << 0,
> +        Floating            = 1 << 1,

Seems like a lot of whitespace again.

> Source/WebCore/display/css/DisplayStyle.h:55
> +    Style(const RenderStyle&);

This should probably be marked explicit.

> Source/WebCore/display/css/DisplayStyle.h:70
> +    void setIsPositioned(bool value) { m_flags.set({ Flags::Positioned }, value); }

Can setters like this be private? Seems like only the constructor really needs to call it (at least so far I guess).

> Source/WebCore/layout/layouttree/LayoutIterator.h:30
> +#include "LayoutInitialContainingBlock.h"

What is requiring this change?

[Simon Fraser (smfr)]

Created attachment 409501 [details]
Patch

[zalan]

Comment on attachment 409501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409501&action=review

> Source/WebCore/ChangeLog:105
> [phone]  Simon Fraser  <[e-mail]>
> +

double changelog.

> Source/WebCore/display/DisplayLayerController.cpp:82
> +    if (!m_displayTree)
>          return;

when is it nullptr?

> Source/WebCore/display/DisplayTreeBuilder.cpp:56
> +    if (!layoutState.hasRoot())
> +        return nullptr;

shouldn't this be an assert instead? -enforcing the caller to only build display tree on a valid layout state?

> Source/WebCore/display/DisplayTreeBuilder.h:58
> +    float m_pixelSnappingFactor;

float m_pixelSnappingFactor { 1 }; ?

> Source/WebCore/display/DisplayView.cpp:91
> +    return page() ? page()->deviceScaleFactor() : 1.0f;

Modern display tree should have a fallback value of 2 device pixels per css pixel. (j/k)

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/267509/webkit

[Radar WebKit Bug Importer]

<rdar://problem/69465821>

[Sam Weinig]

Comment on attachment 409501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409501&action=review

>> Source/WebCore/display/DisplayView.cpp:91
>> +    return page() ? page()->deviceScaleFactor() : 1.0f;
> 
> Modern display tree should have a fallback value of 2 device pixels per css pixel. (j/k)

In what situations does the view need a deviceScaleFactor but not have a page? I guess really the question is why is Page ever null here?

> Source/WebCore/display/css/DisplayCSSPainter.cpp:132
> +        if (image)
> +            context.drawImage(*image, imageRect);

Still curious about why this image can be null. Seems like if we could enforce the invariant that an ImageBox always has an image, this would be simpler.

[Simon Fraser (smfr)]

(In reply to Sam Weinig from comment #9)
> Comment on attachment 409501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409501&action=review
> 
> >> Source/WebCore/display/DisplayView.cpp:91
> >> +    return page() ? page()->deviceScaleFactor() : 1.0f;
> > 
> > Modern display tree should have a fallback value of 2 device pixels per css pixel. (j/k)
> 
> In what situations does the view need a deviceScaleFactor but not have a
> page? I guess really the question is why is Page ever null here?

I null-check it because m_frameView.frame().page() is a pointer. There are some cases involving the page cache where it's null.

Maybe we should just never try to display in these cases?

> > Source/WebCore/display/css/DisplayCSSPainter.cpp:132
> > +        if (image)
> > +            context.drawImage(*image, imageRect);
> 
> Still curious about why this image can be null. Seems like if we could
> enforce the invariant that an ImageBox always has an image, this would be
> simpler.

We could either rebuild part of the display tree when the replaced element's image changes between non-null and null (only using ImageBox when we have an image), or we could keep the same display tree and allow ImageBox a null image. Either could work, but I think the latter is a bit simpler; there may be painting or hit-testing behaviors that we want on ImageBox whether or not it has an image.

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #10)
> (In reply to Sam Weinig from comment #9)
> > Comment on attachment 409501 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=409501&action=review
> > 
> > >> Source/WebCore/display/DisplayView.cpp:91
> > >> +    return page() ? page()->deviceScaleFactor() : 1.0f;
> > > 
> > > Modern display tree should have a fallback value of 2 device pixels per css pixel. (j/k)
> > 
> > In what situations does the view need a deviceScaleFactor but not have a
> > page? I guess really the question is why is Page ever null here?
> 
> I null-check it because m_frameView.frame().page() is a pointer. There are
> some cases involving the page cache where it's null.
> 
> Maybe we should just never try to display in these cases?

Yeah, it would be great if in this new code we could enforce that invariant at the edge, so we don't have to add null checks all over the place. 

> 
> > > Source/WebCore/display/css/DisplayCSSPainter.cpp:132
> > > +        if (image)
> > > +            context.drawImage(*image, imageRect);
> > 
> > Still curious about why this image can be null. Seems like if we could
> > enforce the invariant that an ImageBox always has an image, this would be
> > simpler.
> 
> We could either rebuild part of the display tree when the replaced element's
> image changes between non-null and null (only using ImageBox when we have an
> image), or we could keep the same display tree and allow ImageBox a null
> image. Either could work, but I think the latter is a bit simpler; there may
> be painting or hit-testing behaviors that we want on ImageBox whether or not
> it has an image.

Ok. That makes sense.

[Sam Weinig]

(In reply to Sam Weinig from comment #11)
> (In reply to Simon Fraser (smfr) from comment #10)
> > (In reply to Sam Weinig from comment #9)
> > > Comment on attachment 409501 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=409501&action=review
> > > 
> > > >> Source/WebCore/display/DisplayView.cpp:91
> > > >> +    return page() ? page()->deviceScaleFactor() : 1.0f;
> > > > 
> > > > Modern display tree should have a fallback value of 2 device pixels per css pixel. (j/k)
> > > 
> > > In what situations does the view need a deviceScaleFactor but not have a
> > > page? I guess really the question is why is Page ever null here?
> > 
> > I null-check it because m_frameView.frame().page() is a pointer. There are
> > some cases involving the page cache where it's null.
> > 
> > Maybe we should just never try to display in these cases?
> 
> Yeah, it would be great if in this new code we could enforce that invariant
> at the edge, so we don't have to add null checks all over the place. 

Can we make it so that the Display::View is only ever around when there is a Page? So, if the owning FrameView ever gets disconnected from the Page, we just tear down deallocate Display::View?