[webkit-reviews] review granted: [Bug 203557] Determine viewport distances for lazy image loading : [Attachment 395407] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 3 13:23:05 PDT 2020


Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 203557: Determine viewport distances for lazy image loading
https://bugs.webkit.org/show_bug.cgi?id=203557

Attachment 395407: Patch

https://bugs.webkit.org/attachment.cgi?id=395407&action=review




--- Comment #22 from Darin Adler <darin at apple.com> ---
Comment on attachment 395407
  --> https://bugs.webkit.org/attachment.cgi?id=395407
Patch

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

> Source/WebCore/dom/Document.cpp:7580
> +	       auto intRect = enclosingIntRect(floatRect);

I suggest only putting the height into a local variable since we aren’t using
the location or width:

    auto height = enclosingIntRect(floatRect).height();

> Source/WebCore/dom/Document.cpp:7581
> +	       fprintf(stderr, "Using rect %d\n", intRect.height());

I suspect this printf is something you do not want to check in?

> Source/WebCore/html/LazyLoadImageObserver.cpp:80
>      auto& observer = document.lazyLoadImageObserver();
> -    ASSERT(observer.isObserved(element));
>      observer.m_lazyLoadIntersectionObserver->unobserve(element);

I suggest making it a one-liner.

> Source/WebCore/page/FrameView.cpp:5481
> +    RenderView* renderView = this->renderView();
> +    if (!renderView)
> +	   return FloatRect();
> +
> +    RenderLayerBacking* backing = renderView->layer()->backing();
> +    if (!backing)
> +	   return FloatRect();
> +
> +    auto* graphicsLayer = backing->graphicsLayer();
> +    if (!graphicsLayer)
> +	   return FloatRect();
> +
> +    return graphicsLayer->coverageRect();

My preference is just "auto" here for all these locals, and one word names.
Local variable names like "view", "backing", "layer" don’t need to be two word
phrases in such a short function; the type system will make sure we don’t get
it wrong. Also like { } better than FloatRect().

    auto view = renderView();
    if (!view)
	return { };

Maybe the word "view" isn’t so good?

An advantage of auto is that if we change to return RefPtr we don’t have to
change call sites like these. If we wrote "auto*" then we would. It’s better
not to be so specific; we want to null check something and dereference it, but
we don’t need to constrain its type.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:326
> +    FloatRect coverageRect() const override { return m_coverageRect; }

Can this be final instead?


More information about the webkit-reviews mailing list