[webkit-reviews] review granted: [Bug 157743] Implement ResizeObserver : [Attachment 365042] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted cathiechen
<cathiechen at igalia.com>'s request for review:
Bug 157743: Implement ResizeObserver
https://bugs.webkit.org/show_bug.cgi?id=157743

Attachment 365042: Patch

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




--- Comment #49 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 365042
  --> https://bugs.webkit.org/attachment.cgi?id=365042
Patch

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

Please run the tests with --world-leaks to check whether you've introduced any
new Document leaks.

> Source/WebCore/page/Page.cpp:1308
> +	      
m_documentsNeedingResizeObservationCheck.append(makeWeakPtr(*doc));

Why do you need to store m_documentsNeedingResizeObservationCheck in a member
variable? You could just return it.

> Source/WebCore/page/Page.cpp:1337
> +    // We need layout the whole frame tree here. Because ResizeObserver
could observe element in other frame,

Is that in the spec? https://github.com/w3c/csswg-drafts/issues/3703 hasn't
been commented on.

> Source/WebCore/page/Page.cpp:1340
> +	   mainFrame().view()->updateLayoutAndStyleIfNeededRecursive();

What if this layout...

> Source/WebCore/page/Page.cpp:1343
> +    for (size_t depth = document->gatherResizeObservations(0); depth !=
ResizeObserver::maxElementDepth(); depth =
document->gatherResizeObservations(depth)) {

Nulled out document

> Source/WebCore/page/ResizeObservation.h:46
> +    LayoutSize computeObservationSize() const;

Maybe computeObservedSize