[webkit-reviews] review denied: [Bug 157743] Implement ResizeObserver : [Attachment 362001] Change LayoutSize(0, 0) to LayoutSize()

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 157743: Implement ResizeObserver
https://bugs.webkit.org/show_bug.cgi?id=157743

Attachment 362001: Change LayoutSize(0, 0) to LayoutSize()

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




--- Comment #38 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 362001
  --> https://bugs.webkit.org/attachment.cgi?id=362001
Change LayoutSize(0, 0) to LayoutSize()

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

I don't think the layout hooks are right in this patch, since you're going to
fire resize observations after every layout, which could be much higher than
requestAnimationFrame frequency.

What's the expected behavior if a layout resizes an element, then a second
forced layout puts it back at the original size, and then rAF fires? Should
observations fire?

> Source/WebCore/dom/Document.cpp:8104
> +	   if (d < minDepth)
> +	       minDepth = d;

minDepth = min(d, minDepth)

> Source/WebCore/dom/Document.h:1434
> +    size_t gatherResizeObservations(size_t);

Name the parameter (it's not obvious). Add a comment saying what the return
value means.

> Source/WebCore/dom/Document.h:1436
> +    void deliverObservations();
> +    bool skippedObservations();

These need the word "resize" in the name: deliverResizeObservations() etc.

skippedObservations() doesn't follow our naming conventions for things that
return bool. hasSkippedResizeObserver() const. Or a better name would help me
understand what "skipped" means here.

> Source/WebCore/dom/Document.h:1437
> +    void scheduleResizeObserveIfNeeded();

"scheduleResizeObserve" is not the best name. scheduleResizeObservations?
scheduleResizeObservers?

> Source/WebCore/dom/ElementRareData.h:172
> +	   if (m_resizeObserverData)
> +	       result.add(UseType::ResizeObserver);

This is missing the #if guards.

> Source/WebCore/dom/ScriptedAnimationController.cpp:296
> +    // TODO(cathiechen): scheduleResizeObserveIfNeeded here.

Please watch bug 177484.

> Source/WebCore/page/FrameViewLayoutContext.cpp:146
> +#if ENABLE(RESIZE_OBSERVER)
> +    , m_resizeObserverTimer(*this,
&FrameViewLayoutContext::resizeObserverTimerFired)
> +#endif

I don't think you should put resize observer stuff into FrameViewLayoutContext;
I think it should be moved out to FrameView.

> Source/WebCore/page/FrameViewLayoutContext.cpp:231
> +    notifyResizeObservers(layoutRoot);

So this can trigger extra layouts, and then call out to script. I don't think
we can allow that here; this classs assumes that no script runs until
post-layout tasks run.

> Source/WebCore/page/ResizeObservation.cpp:42
> +    , m_observationSize(0, 0)

No need to initialize this.

> Source/WebCore/page/ResizeObservation.cpp:70
> +	   return FloatRect(box->paddingLeft(), box->paddingTop(),
m_observationSize.width(), m_observationSize.height());

Why is this not just calling contentBoxRect()?

I'm not sure why this function and the one above don't share code.

> Source/WebCore/page/ResizeObservation.cpp:75
> +bool ResizeObservation::elementSizeChanged()

This function fetches the new size to compare with the old (but doesn't store
the new size). Then the caller fetches the new size again by calling
updateObservationSize(), so this could be optimized.

> Source/WebCore/page/ResizeObservation.cpp:84
> +	   if (m_observationSize == LayoutSize())

m_observationSize.isZero()

> Source/WebCore/page/ResizeObservation.cpp:91
> +size_t ResizeObservation::targetDepth()

This name is ambiguous. Is it the depth of the target element, or the depth
that is trying to be reached?

> Source/WebCore/page/ResizeObservation.cpp:95
> +    for (Element* parent = m_target; parent; parent =
parent->parentElement())
> +	   ++depth;

Do we really have to do this parent walk every time? These things typically
show up on profiles.

> Source/WebCore/page/ResizeObservation.h:61
> +    // target size in last observation
> +    LayoutSize m_observationSize;

So rename the member variable to avoid the need for the comment?

> Source/WebCore/page/ResizeObserver.h:60
> +    static size_t depthBottom() { return 4096; }

Bad name, and where does the 4096 come from?

> Source/WebCore/page/ResizeObserver.h:83
> +    bool m_skippedObservations { false };

m_hasSkippedObservations ?