Bug 157743 – Implement ResizeObserver

• Component: DOM
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

https://wicg.github.io/ResizeObserver/

[Radar WebKit Bug Importer]

<rdar://problem/26302223>

[Philip Jägenstedt]

There's an Intent to Ship for this on blink-dev:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/z6ienONUb5A/GlHRa9S2AAAJ

Please shout if shipping this now is not a good idea.

[William J. Edney]

This has now landed in Chrome 64: https://bugs.chromium.org/p/chromium/issues/detail?id=612962

It'd be super cool to get this in Webkit/Safari :-).

Thanks!

[martin]

This is extreme helpful for developers (https://developers.google.com/web/updates/2016/10/resizeobserver). Unfortunately polyfills are not enough here. Please provide support for this functionality.

[Eric G]

Would love to see this feature.

[Eric G]

Probably should be mentioned on https://webkit.org/status/

[cathiechen]

Hi, Eric.

I'm working on this. Sorry, I didn't mention it on https://webkit.org/status/

The first patch would be on the way. Welcome to do the code review. Thanks:)

[cathiechen]

Created attachment 359876 [details]
Data structure and management

[Ali Juma]

Comment on attachment 359876 [details]
Data structure and management

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

Thanks for working on this!

Once you're ready to start landing patches, it probably makes sense to open a dependent bug for each patch. Are there web-platform-tests for ResizeObserver that we can import? Then you can update their results as you start landing patches, and it becomes easier to see how each patch brings us closer to a complete correct implementation.

> Source/WebCore/dom/Element.cpp:1848
> +                newDocument.addResizeObserver(*observer);

Is this moving of observers needed for ResizeObserver? It's needed for IntersectionObserver to handle observers for which this Element is the root element. But ResizeObservers don't have a root element; instead, they're always associated with a particular Document.

> Source/WebCore/page/ResizeObserver.cpp:67
> +    m_implicitRootDocument->addResizeObserver(*this);

Do we need to check if the document already knows about this observer? (i.e., if we already had targets before this call to ::observer, the document already knows about the observer)

> Source/WebCore/page/ResizeObserver.cpp:140
> +    }

Can we use m_observations.removeFirstMatching(.....) here instead of writing our own loop?

> Source/WebCore/page/ResizeObserver.h:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

For all the new files you're adding, you'll want to use your own company's name here rather than Apple.

> Source/WebKit/Shared/WebPreferences.yaml:1549
> +  defaultValue: true

The default should be false for now, given that the implementation isn't complete.

> Source/cmake/WebKitFeatures.cmake:180
> +    WEBKIT_OPTION_DEFINE(ENABLE_RESIZE_OBSERVER "Enable Resize Observer support" PRIVATE ON)

You'll also need to enable this for the Xcode build in the various .xcconfig files (e.g., grep for 'ENABLE_INTERSECTION_OBSERVER' to see all the places where this is needed).

[Ali Juma]

Comment on attachment 359876 [details]
Data structure and management

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

One more comment:

> Source/WebCore/page/ResizeObserver.h:73
> +    WeakPtr<Document> m_implicitRootDocument;

This can just be m_document, since for ResizeObserver we don't have the notions of 'explicit' and 'implicit' roots.

[Aleksandar Totic]

FYI: 

Chrome is currently working on extending ResizeObserver API to support observation of different CSS boxes. The new draft spec is at:

https://drafts.csswg.org/resize-observer-1/

We'll be talking about it at CSSWG meet in SF in February, and Chrome hopes
to ship it this year.

Main change is that:

ResizeObserver::observe(Element) becomes:
ResizeObserver::observe(Element, { box: "border-box" | "content-box" | "scroll-box" | "device-pixel-border-box" }

And ResizeObserverEntry gets new boxes:

interface ResizeObserverEntry {
    readonly attribute Element target;
    readonly attribute DOMRectReadOnly contentRect;
    readonly attribute ResizeObserverSize borderBoxSize;
    readonly attribute ResizeObserverSize contentSize;
    readonly attribute ResizeObserverSize scrollSize;
    readonly attribute ResizeObserverSize devicePixelBorderBoxSize;
}

If you have any feedback on the new spec, we'd love to hear it on 

https://github.com/w3c/csswg-drafts/issues

[Simon Fraser (smfr)]

(In reply to Aleksandar Totic from comment #11)
> 
> If you have any feedback on the new spec, we'd love to hear it on 
> 
> https://github.com/w3c/csswg-drafts/issues

Our main concern with the spec is the O(N) layouts (where N is deepest node depth) when firing observations.

[Aleksandar Totic]

> Our main concern with the spec is the O(N) layouts (where N is deepest node depth) when firing observations.

That is the upper bound, when every callback modifies its children. Hopefully, the real world usage will be closer to O(2), 1 layout before notifications, and 1 after.

[Ryosuke Niwa]

(In reply to Aleksandar Totic from comment #13)
> > Our main concern with the spec is the O(N) layouts (where N is deepest node depth) when firing observations.
> 
> That is the upper bound, when every callback modifies its children.
> Hopefully, the real world usage will be closer to O(2), 1 layout before
> notifications, and 1 after.

Given how often we see O(n^2) behaviors with layout triggering more layouts in real websites, I remain highly skeptical with such an optimistic proposition.

[cathiechen]

Comment on attachment 359876 [details]
Data structure and management

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

Hi Ali,
Thanks for the swift review.

>> Source/WebCore/dom/Element.cpp:1848
>> +                newDocument.addResizeObserver(*observer);
> 
> Is this moving of observers needed for ResizeObserver? It's needed for IntersectionObserver to handle observers for which this Element is the root element. But ResizeObservers don't have a root element; instead, they're always associated with a particular Document.

Yes, I think we should disconnectFromResizeObservers() here in stead. If element is moved to another document, it should be removed from ResizeObserver of this document.

>> Source/WebCore/page/ResizeObserver.cpp:67
>> +    m_implicitRootDocument->addResizeObserver(*this);
> 
> Do we need to check if the document already knows about this observer? (i.e., if we already had targets before this call to ::observer, the document already knows about the observer)

Yes, we should. Done!

>> Source/WebCore/page/ResizeObserver.cpp:140
>> +    }
> 
> Can we use m_observations.removeFirstMatching(.....) here instead of writing our own loop?

Done

>> Source/WebCore/page/ResizeObserver.h:2
>> + * Copyright (C) 2019 Apple Inc. All rights reserved.
> 
> For all the new files you're adding, you'll want to use your own company's name here rather than Apple.

Done

>> Source/WebCore/page/ResizeObserver.h:73
>> +    WeakPtr<Document> m_implicitRootDocument;
> 
> This can just be m_document, since for ResizeObserver we don't have the notions of 'explicit' and 'implicit' roots.

Done

>> Source/WebKit/Shared/WebPreferences.yaml:1549
>> +  defaultValue: true
> 
> The default should be false for now, given that the implementation isn't complete.

Done

>> Source/cmake/WebKitFeatures.cmake:180
>> +    WEBKIT_OPTION_DEFINE(ENABLE_RESIZE_OBSERVER "Enable Resize Observer support" PRIVATE ON)
> 
> You'll also need to enable this for the Xcode build in the various .xcconfig files (e.g., grep for 'ENABLE_INTERSECTION_OBSERVER' to see all the places where this is needed).

OK, will do. Thanks!

[cathiechen]

Created attachment 360287 [details]
WIP-002

[cathiechen]

Created attachment 360290 [details]
WIP-003

[cathiechen]

Created attachment 360291 [details]
WIP-004

[Frédéric Wang (:fredw)]

Comment on attachment 360291 [details]
WIP-004

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

> Source/WebKit/Shared/WebPreferences.yaml:1563
> +ResizeObserverEnabled:

It seems you have a duplicate entry for ResizeObserverEnabled:

[cathiechen]

Comment on attachment 360291 [details]
WIP-004

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

>> Source/WebKit/Shared/WebPreferences.yaml:1563
>> +ResizeObserverEnabled:
> 
> It seems you have a duplicate entry for ResizeObserverEnabled:

Ah, thanks! I'll look into it.

[cathiechen]

Created attachment 361796 [details]
WIP-with-mainly-implement-except-svg-support

This is a wip patch.

The current status:
- Svg is not supported yet.
- The reason to cause eventloop.html fail is that the rafCount in test(0) is request the trigger of resizeObserver corresponding with RequestAnimationFrame, but it is triggered by timer now. Other cases in eventloop.html are good.
- Passed observe.html. test7 is testing `this` in callback function. This failure isn't specific in resizeOberver. So low the priority.
- Passed notify.html

[Rob Buis]

Comment on attachment 361796 [details]
WIP-with-mainly-implement-except-svg-support

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

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

Spacing is wrong. I wonder if check-webkit-style catches this.

[cathiechen]

Created attachment 361805 [details]
Fixed the compile issue after rebase

Fixed the compile issue after rebase.

1. Fixed the include issue in ScrollingStateFrameHostingNode.cpp/.h. I will file a bug to this separately.
2. ActiveDOMObject(document) is deleted.

[cathiechen]

(In reply to Rob Buis from comment #22)
> Comment on attachment 361796 [details]
> WIP-with-mainly-implement-except-svg-support
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361796&action=review
> 
> > Source/WebCore/page/ResizeObserver.h:64
> > +    static size_t depthBottom() { return 4096;}
> 
> Spacing is wrong. I wonder if check-webkit-style catches this.

Done! Thanks:)

[cathiechen]

Created attachment 361897 [details]
Supported svg element

Passed svg.html

[Rob Buis]

Comment on attachment 361897 [details]
Supported svg element

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

This patch seems to do two things, import tests and add a runtime flag. Given the bug title, it seems at a minimum the ChangeLog(s) should explain this.

> ChangeLog:1
> [phone]  cathie chen  <[e-mail]>

Optional: it is usual to start names with a capital.

> Source/JavaScriptCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Are you sure this change is needed in JSC?

> Source/WebCore/PAL/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=157743

Are you sure this is needed in PAL?

[cathiechen]

Created attachment 361903 [details]
Patch contains all changes

Sorry, the previous patch didn't include all changes.

[cathiechen]

> This patch seems to do two things, import tests and add a runtime flag.
> Given the bug title, it seems at a minimum the ChangeLog(s) should explain
> this.

Sorry the previous patch is incomplete. It has been updated.

> > ChangeLog:1
> > [phone]  cathie chen  <[e-mail]>
> 
> Optional: it is usual to start names with a capital.
> 
> > Source/JavaScriptCore/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> Are you sure this change is needed in JSC?
> 
> > Source/WebCore/PAL/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=157743
> 
> Are you sure this is needed in PAL?

In order to less confuse, I removed the ChangLogs temporally.

[Rob Buis]

Comment on attachment 361903 [details]
Patch contains all changes

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

Looks good on first glance.

> Source/WebCore/page/ResizeObservation.cpp:36
> +    return WTF::adoptRef(*new ResizeObservation(target));

WTF:: prefix is not needed.

> Source/WebCore/page/ResizeObservation.cpp:59
> +        downcast<SVGElement>(*m_target).getBoundingBox(svgRect);

Have you tried getting the rect through the render tree, like for non SVG?

> Source/WebCore/page/ResizeObservation.cpp:71
> +    if (!m_target->isSVGElement() && m_target->renderBox())

This should reuse the box variable. Also what happens if you do not check for non SVG?

> Source/WebCore/page/ResizeObservation.h:60
> +    bool m_elmentSizeChanged;

Could initialize more here instead of in initializer list.

> Source/WebCore/page/ResizeObserver.cpp:38
> +    return WTF::adoptRef(*new ResizeObserver(document, WTFMove(callback)));

WTF:: prefix is not needed.

> Source/WebCore/page/ResizeObserver.cpp:44
> +    , m_skippedObservations(false)

This can be done in the header as well, personally I prefer that.

> Source/WebCore/page/ResizeObserver.cpp:46
> +    m_document = makeWeakPtr(document);

I think this can go into the constructor list.

> Source/WebCore/page/ResizeObserverEntry.h:44
> +        return WTF::adoptRef(*new ResizeObserverEntry(target, contentRect));

WTF:: prefix is not needed.

[Rob Buis]

Comment on attachment 361903 [details]
Patch contains all changes

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

Found some more stuff.

> Source/WebCore/page/ResizeObservation.cpp:64
> +        setObservationSize(LayoutSize(0, 0));

The default constructor defaults to (0, 0), I think you can just use that.

> Source/WebCore/page/ResizeObservation.cpp:83
> +    } else if (!m_target->renderBox()) {

Probably check-webkit-style catches this, but if the if block returns, no need for an else.

> Source/WebCore/page/ResizeObserver.cpp:103
> +bool ResizeObserver::removeTarget(Element& target)

Could target be const or does it need to be changed?

> Source/WebCore/page/ResizeObserverEntry.h:47
> +    Element* target() { return m_target.get(); }

Can be const.

> Source/WebCore/page/ResizeObserverEntry.h:48
> +    DOMRectReadOnly* contentRect() { return m_contentRect.get(); }

Ditto. In general if in the chromium resize observerimplementation const is used, we can probably introduce it here too.

> Source/WebCore/page/ResizeObserverEntry.h:54
> +        m_contentRect = DOMRectReadOnly::create(contentRect.x(), contentRect.y(), contentRect.width(), contentRect.height());

I don't see why this could not be in the initializer list. However since it same in chromium implementation, that is optional.

> Source/WebCore/page/Settings.yaml:820
> +  inspectorOverride: true

This change seems unrelated.

[cathiechen]

Comment on attachment 361903 [details]
Patch contains all changes

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

Hi Rob,
Thanks for the review:)

>> Source/WebCore/page/ResizeObservation.cpp:36
>> +    return WTF::adoptRef(*new ResizeObservation(target));
> 
> WTF:: prefix is not needed.

Done

>> Source/WebCore/page/ResizeObservation.cpp:59
>> +        downcast<SVGElement>(*m_target).getBoundingBox(svgRect);
> 
> Have you tried getting the rect through the render tree, like for non SVG?

I looked into RenderSVGModelObject, but not all svg object is inherited from it.
Acorrding to https://drafts.csswg.org/resize-observer-1/ `3.4.7. Calculate box size, given target and specific size`
SVGElement::getBoundingBox could deal with SVGGraphicsElement correctly.
Thanks I will add a TODO to deal with !SVGGraphicsElement

>> Source/WebCore/page/ResizeObservation.cpp:64
>> +        setObservationSize(LayoutSize(0, 0));
> 
> The default constructor defaults to (0, 0), I think you can just use that.

If the display of m_target is changed from "block", we still need set the size to (0, 0)

>> Source/WebCore/page/ResizeObservation.cpp:71
>> +    if (!m_target->isSVGElement() && m_target->renderBox())
> 
> This should reuse the box variable. Also what happens if you do not check for non SVG?

box: Done.

RenderSVGBlock is inherited from RenderBlockFlow

>> Source/WebCore/page/ResizeObservation.cpp:83
>> +    } else if (!m_target->renderBox()) {
> 
> Probably check-webkit-style catches this, but if the if block returns, no need for an else.

Done

>> Source/WebCore/page/ResizeObservation.h:60
>> +    bool m_elmentSizeChanged;
> 
> Could initialize more here instead of in initializer list.

Done

>> Source/WebCore/page/ResizeObserver.cpp:38
>> +    return WTF::adoptRef(*new ResizeObserver(document, WTFMove(callback)));
> 
> WTF:: prefix is not needed.

Done

>> Source/WebCore/page/ResizeObserver.cpp:44
>> +    , m_skippedObservations(false)
> 
> This can be done in the header as well, personally I prefer that.

Done

>> Source/WebCore/page/ResizeObserver.cpp:46
>> +    m_document = makeWeakPtr(document);
> 
> I think this can go into the constructor list.

Done

>> Source/WebCore/page/ResizeObserver.cpp:103
>> +bool ResizeObserver::removeTarget(Element& target)
> 
> Could target be const or does it need to be changed?

Yes, logically this function might remove observer from target.resizeObserverData()->observers.

While the target in removeObservation should be const.

>> Source/WebCore/page/ResizeObserverEntry.h:44
>> +        return WTF::adoptRef(*new ResizeObserverEntry(target, contentRect));
> 
> WTF:: prefix is not needed.

Done

>> Source/WebCore/page/ResizeObserverEntry.h:47
>> +    Element* target() { return m_target.get(); }
> 
> Can be const.

Done

>> Source/WebCore/page/ResizeObserverEntry.h:48
>> +    DOMRectReadOnly* contentRect() { return m_contentRect.get(); }
> 
> Ditto. In general if in the chromium resize observerimplementation const is used, we can probably introduce it here too.

Done

>> Source/WebCore/page/ResizeObserverEntry.h:54
>> +        m_contentRect = DOMRectReadOnly::create(contentRect.x(), contentRect.y(), contentRect.width(), contentRect.height());
> 
> I don't see why this could not be in the initializer list. However since it same in chromium implementation, that is optional.

Done

>> Source/WebCore/page/Settings.yaml:820
>> +  inspectorOverride: true
> 
> This change seems unrelated.

Done

[cathiechen]

Created attachment 361999 [details]
Fixed the review issues

Fixed the review issues.

[cathiechen]

Created attachment 362000 [details]
Fixed the review issues

Fixed the orders in construct.

[cathiechen]

Created attachment 362001 [details]
Change LayoutSize(0, 0) to LayoutSize()

Change LayoutSize(0, 0) to LayoutSize()

[cathiechen]

> >> Source/WebCore/page/ResizeObservation.cpp:64
> >> +        setObservationSize(LayoutSize(0, 0));
> > 
> > The default constructor defaults to (0, 0), I think you can just use that.
> 
> If the display of m_target is changed from "block", we still need set the
> size to (0, 0)
> 

Sorry I misunderstood it! Changed LayoutSize(0, 0) to LayoutSize()

[Rob Buis]

Comment on attachment 362001 [details]
Change LayoutSize(0, 0) to LayoutSize()

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

Found some more things.

> Source/WebCore/page/FrameViewLayoutContext.cpp:52
> +#include <JavaScriptCore/ScriptCallStack.h>

Is this include needed?

> Source/WebCore/page/FrameViewLayoutContext.cpp:665
> +        layoutRoot->layout();

Why is this needed here? Isn't the same code in the for loop enough?

> Source/WebCore/page/FrameViewLayoutContext.cpp:680
> +        document()->reportException("ResizeObserver loop completed with undelivered notifications.", line, column, url, nullptr, 0);

Better to use nullptr instead of 0 for last parameter.

> Source/WebCore/page/ResizeObservation.h:52
> +    Element* target() { return m_target; }

This can be const. In general if in the chromium resize observerimplementation const is used, we can probably introduce it here too.

> Source/WebCore/page/ResizeObservation.h:57
> +

One empty line too much here.

> Source/WebCore/page/ResizeObserver.cpp:157
> +    return ret;

Looks like temp varret will not be needed.

> Source/WebCore/page/ResizeObserverEntry.h:47
> +    const Element* target() { return m_target.get(); }

The const should probably be on the method?

> Source/WebCore/page/ResizeObserverEntry.h:48
> +    const DOMRectReadOnly* contentRect() { return m_contentRect.get(); }

Ditto.

> Source/cmake/WebKitFeatures.cmake:187
> +    WEBKIT_OPTION_DEFINE(ENABLE_RESIZE_OBSERVER "Enable Resize Observer support" PRIVATE ON)

I guess default should be off at first.

[alan baradlay]

Comment on attachment 362001 [details]
Change LayoutSize(0, 0) to LayoutSize()

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

>> Source/WebCore/page/FrameViewLayoutContext.cpp:665
>> +        layoutRoot->layout();
> 
> Why is this needed here? Isn't the same code in the for loop enough?

I assume you have to have a clean tree to call out to deliverObservations(). (this covers the case when the notifyResizeObservers gets called through the timer firing)

[Simon Fraser (smfr)]

Comment on attachment 362001 [details]
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 ?

[cathiechen]

Comment on attachment 362001 [details]
Change LayoutSize(0, 0) to LayoutSize()

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

>> Source/WebCore/page/FrameViewLayoutContext.cpp:52
>> +#include <JavaScriptCore/ScriptCallStack.h>
> 
> Is this include needed?

Yes, document()->reportException(...) defined in ScriptExecutionContext.h need this #include.
The callstack is set to 0.

>>> Source/WebCore/page/FrameViewLayoutContext.cpp:665
>>> +        layoutRoot->layout();
>> 
>> Why is this needed here? Isn't the same code in the for loop enough?
> 
> I assume you have to have a clean tree to call out to deliverObservations(). (this covers the case when the notifyResizeObservers gets called through the timer firing)

Yes, exactly like zalan@ said, I need a clean tree here.

>> Source/WebCore/page/FrameViewLayoutContext.cpp:680
>> +        document()->reportException("ResizeObserver loop completed with undelivered notifications.", line, column, url, nullptr, 0);
> 
> Better to use nullptr instead of 0 for last parameter.

Done

>> Source/WebCore/page/ResizeObservation.h:52
>> +    Element* target() { return m_target; }
> 
> This can be const. In general if in the chromium resize observerimplementation const is used, we can probably introduce it here too.

Done

>> Source/WebCore/page/ResizeObservation.h:57
>> +
> 
> One empty line too much here.

Done

>> Source/WebCore/page/ResizeObserver.cpp:157
>> +    return ret;
> 
> Looks like temp varret will not be needed.

Done

>> Source/WebCore/page/ResizeObserverEntry.h:47
>> +    const Element* target() { return m_target.get(); }
> 
> The const should probably be on the method?

Done

>> Source/WebCore/page/ResizeObserverEntry.h:48
>> +    const DOMRectReadOnly* contentRect() { return m_contentRect.get(); }
> 
> Ditto.

Done

>> Source/cmake/WebKitFeatures.cmake:187
>> +    WEBKIT_OPTION_DEFINE(ENABLE_RESIZE_OBSERVER "Enable Resize Observer support" PRIVATE ON)
> 
> I guess default should be off at first.

Done

[cathiechen]

Comment on attachment 362001 [details]
Change LayoutSize(0, 0) to LayoutSize()

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

Hi Simon,
Thanks for the review and sorry for the delay.

Yes, I agree that the layout hooks would bring extra fires. So I moved it to Page::willDisplayPage, like intersectionObserver.
Would this change make sense to you?

Sorry, the reply is long. I tried to explain them clearly. Please let me know if I misunderstood anything. Thanks:)

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

Done

>> 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.

Done

>> Source/WebCore/dom/Document.h:1436
>> +    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.

Done

>> Source/WebCore/dom/Document.h:1437
>> +    void scheduleResizeObserveIfNeeded();
> 
> "scheduleResizeObserve" is not the best name. scheduleResizeObservations? scheduleResizeObservers?

Done. Use scheduleResizeObservations.

>> Source/WebCore/dom/ElementRareData.h:172
>> +            result.add(UseType::ResizeObserver);
> 
> This is missing the #if guards.

Done

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

Great! Based on 177484, notifyResizeObservers would be able to keep synchronicity with requestAnimationFrame.

>> Source/WebCore/page/FrameViewLayoutContext.cpp:146
>> +#endif
> 
> I don't think you should put resize observer stuff into FrameViewLayoutContext; I think it should be moved out to FrameView.

Done. The timer has been moved to Page.

>> 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.

Done. This has been removed. notifyResizeObservers() would be called in Page::willDisplayPage().

>> Source/WebCore/page/ResizeObservation.cpp:42
>> +    , m_observationSize(0, 0)
> 
> No need to initialize this.

Done

>> 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.

Done with share code.

Regarding contentBoxRect():
https://drafts.csswg.org/resize-observer/#content-rect-h
According to the current status of the spec the content rect is (paddingLeft, paddingTop, contentWidth, contentHeight). Border and scrollbar width are not included.
I noticed that this is still in discuss in csswg. Maybe we could bring this on csswg discuss?

>> 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.

Done. Fetch the size and store it. Reuse it if updateObservationSize().

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

Done

>> 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?

Done. Changed to targetElementDepth.

>> Source/WebCore/page/ResizeObservation.cpp:95
>> +        ++depth;
> 
> Do we really have to do this parent walk every time? These things typically show up on profiles.

I'm afraid so. The depth is not easy to maintain when DOM tree changed.
However, this is called in the constraint of elementSizeChanged. The trigger time would be 1 in common case. If it has been skipped, the tigger time would be 1+notify time.

>> Source/WebCore/page/ResizeObservation.h:61
>> +    LayoutSize m_observationSize;
> 
> So rename the member variable to avoid the need for the comment?

Done. Changed to m_lastObservationSize.

>> Source/WebCore/page/ResizeObserver.h:60
>> +    static size_t depthBottom() { return 4096; }
> 
> Bad name, and where does the 4096 come from?

How about maxElementDepth()?
Sorry, this is borrowed from Chromium. It is ∞ in the spec, so changed it to SIZE_MAX.

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

Done

[cathiechen]

Created attachment 363087 [details]
Fixed the review issues

DONE
- Support idl_test
- notifyResizeObservers() in willDisplayPage.
- Compute observerSize once and reuse it when need update.
- Deal with !SVGGraphicsElement's size.

TODO
- Introduce a flag to indicate whether element's size is changed.
- Settings.

[cathiechen]

Created attachment 363219 [details]
m_needsCheckResizeObservations

Add m_needsCheckResizeObservations to reduce notifyResizeObservers().

[cathiechen]

Created attachment 363505 [details]
Patch

[cathiechen]

Created attachment 363506 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 363506 [details]
Patch

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

Do resizeObserver callbacks get correctly labelled in Web Inspector timeltines? Please test.

> Source/WebCore/dom/Document.cpp:8075
> +        auto d = observer->gatherObservations(deeperThan);

d -> depth

> Source/WebCore/dom/Document.h:1344
> +    void getParserLocation(String& url, unsigned& line, unsigned& column);

Can the method be const?

> Source/WebCore/dom/Document.h:1415
> +    // Return the minDepth of the active observations

Comments should end with a period.

> Source/WebCore/dom/Element.cpp:1868
> +    disconnectFromResizeObservers();

Does resizeObserver allow you to observe an element in a different document, like IntersectionObserver does? If so, this code might need to change.

There should be a test that tries to do this.
https://github.com/WICG/ResizeObserver/issues/64

> Source/WebCore/page/FrameView.cpp:5375
> +        doc->deliverResizeObservations();

This looks dangerous; what if the observation destroys the frame it's in, potentially destroying the document? There should be a test that does this.

> Source/WebCore/page/Page.cpp:1314
> +    for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) {
> +        if (FrameView* frameView = frame->view())
> +            frameView->notifyResizeObservers();
> +    }

notifyResizeObservers() can run script, so mutate the frame tree. This doesn't look safe.

> Source/WebCore/page/ResizeObservation.cpp:73
> +    return LayoutPoint();

return { };

> Source/WebCore/page/ResizeObserver.cpp:52
> +    m_document->removeResizeObserver(*this);

m_document is a WeakPtr, so how do you know it's non-null here?

[cathiechen]

Created attachment 365042 [details]
Patch

[EWS Watchlist]

Comment on attachment 365042 [details]
Patch

Attachment 365042 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11553231

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365051 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Simon Fraser (smfr)]

Comment on attachment 365042 [details]
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

[EWS Watchlist]

Comment on attachment 365042 [details]
Patch

Attachment 365042 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11553307

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365064 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[cathiechen]

Created attachment 365112 [details]
Support multi frames

- Support multi frames.

[EWS Watchlist]

Comment on attachment 365112 [details]
Support multi frames

Attachment 365112 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11558127

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365120 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365112 [details]
Support multi frames

Attachment 365112 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11558881

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365126 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365112 [details]
Support multi frames

Attachment 365112 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11559228

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365128 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365112 [details]
Support multi frames

Attachment 365112 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11559331

New failing tests:
resize-observer/multi-frames.html
resize-observer/modify-frametree-in-callback.html

[EWS Watchlist]

Created attachment 365133 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[cathiechen]

Created attachment 365144 [details]
troggle ENABLE(RESIZE_OBSERVER)

[cathiechen]

Created attachment 365145 [details]
Toggle ENABLE(RESIZE_OBSERVER) and fix ios build issue

[EWS Watchlist]

Comment on attachment 365145 [details]
Toggle ENABLE(RESIZE_OBSERVER) and fix ios build issue

Attachment 365145 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11562671

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365146 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365145 [details]
Toggle ENABLE(RESIZE_OBSERVER) and fix ios build issue

Attachment 365145 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11562932

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365148 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365145 [details]
Toggle ENABLE(RESIZE_OBSERVER) and fix ios build issue

Attachment 365145 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11563379

New failing tests:
resize-observer/multi-frames.html
resize-observer/modify-frametree-in-callback.html

[EWS Watchlist]

Created attachment 365149 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[cathiechen]

Created attachment 365171 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error

[EWS Watchlist]

Comment on attachment 365171 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error

Attachment 365171 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11567400

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365187 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365171 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error

Attachment 365171 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11567576

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365191 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365171 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error

Attachment 365171 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11568183

New failing tests:
resize-observer/multi-frames.html
resize-observer/modify-frametree-in-callback.html

[EWS Watchlist]

Created attachment 365209 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[cathiechen]

Created attachment 365285 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error

[EWS Watchlist]

Comment on attachment 365285 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error

Attachment 365285 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11574761

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365301 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365285 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error

Attachment 365285 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11575430

New failing tests:
resize-observer/multi-frames.html
resize-observer/modify-frametree-in-callback.html

[EWS Watchlist]

Created attachment 365315 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 365285 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error

Attachment 365285 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11575588

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365319 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[cathiechen]

Comment on attachment 365042 [details]
Patch

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

Hi Simon,

Thanks for the swift review! :)

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

Checked in my local env, no leak report. :)

>> 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.

Done.

>> 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.

Not yet. But I think across-document is worthy to be supported. I would add some test cases for this issue later. :)

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

notifyResizeObservers() could be invoked by m_resizeObserverTimer. We need to make sure layout is clean here.

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

gatherResizeObservations() won't invoke js, so I think it's safe to use document.
deliverResizeObservations() will invoke js, so I checked document after it.

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

Done.

[cathiechen]

Created attachment 365349 [details]
Fixed code review issues and remove ENABLE_INTERSECTION_OBSERVER

ENABLE_INTERSECTION_OBSERVER is removed, because:
1. It would be set to true by default.
2. We have a runtime flag for ResizeObserver.
3. It's tricky to toggle ENABLE_INTERSECTION_OBSERVER in ews.
4. This patch becomes smaller. :)

[Simon Fraser (smfr)]

Please keep ENABLE_INTERSECTION_OBSERVER (sorry!). Not all ports may want to compile this in.

[cathiechen]

> Please keep ENABLE_INTERSECTION_OBSERVER (sorry!). Not all ports may want to compile this in.

Ah, OK, thanks for pointing this out. I'm gonna upload a patch without ENABLE_INTERSECTION_OBSERVER to confirm if the failure of ews is related to it.
Then I'll add it back.

[cathiechen]

Created attachment 365479 [details]
Patch

[EWS Watchlist]

Comment on attachment 365479 [details]
Patch

Attachment 365479 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11592366

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365487 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365479 [details]
Patch

Attachment 365479 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11592505

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365493 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[cathiechen]

Created attachment 365494 [details]
Patch

[EWS Watchlist]

Comment on attachment 365494 [details]
Patch

Attachment 365494 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11594154

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365510 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365494 [details]
Patch

Attachment 365494 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11594461

New failing tests:
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
resize-observer/multi-frames.html
imported/w3c/web-platform-tests/resize-observer/observe.html
resize-observer/modify-frametree-in-callback.html
imported/w3c/web-platform-tests/resize-observer/svg.html
imported/w3c/web-platform-tests/resize-observer/eventloop.html

[EWS Watchlist]

Created attachment 365518 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365494 [details]
Patch

Attachment 365494 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11595115

New failing tests:
resize-observer/modify-frametree-in-callback.html
resize-observer/multi-frames.html

[EWS Watchlist]

Created attachment 365522 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[cathiechen]

Created attachment 365702 [details]
Patch

[cathiechen]

- Add test case resize-observer/observe-element-from-other-frame.html
- Deal with EWS WK1 error.

[cathiechen]

Created attachment 365711 [details]
Patch

[EWS Watchlist]

Comment on attachment 365711 [details]
Patch

Attachment 365711 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11612440

New failing tests:
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/svg.html

[EWS Watchlist]

Created attachment 365717 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365711 [details]
Patch

Attachment 365711 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11612525

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365720 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365711 [details]
Patch

Attachment 365711 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11613746

New failing tests:
imported/w3c/web-platform-tests/resize-observer/idlharness.window.html
imported/w3c/web-platform-tests/resize-observer/notify.html
imported/w3c/web-platform-tests/resize-observer/svg.html

[EWS Watchlist]

Created attachment 365737 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[cathiechen]

Created attachment 365953 [details]
Patch

[cathiechen]

Created attachment 365954 [details]
Patch

[cathiechen]

Created attachment 365969 [details]
Patch

[cathiechen]

Created attachment 365972 [details]
Patch

[EWS Watchlist]

Comment on attachment 365972 [details]
Patch

Attachment 365972 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11675658

New failing tests:
fast/visual-viewport/ios/min-scale-greater-than-one.html

[EWS Watchlist]

Created attachment 366015 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[cathiechen]

Created attachment 366064 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 366064 [details]
Patch

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

> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:227
> +    HRESULT setResizeObserverEnabled([in] BOOL enabled);

I think this should be added to the latest version, so IWebPreferencesPrivate7

[cathiechen]

Created attachment 366073 [details]
Patch

[cathiechen]

Comment on attachment 366064 [details]
Patch

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

>> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:227
>> +    HRESULT setResizeObserverEnabled([in] BOOL enabled);
> 
> I think this should be added to the latest version, so IWebPreferencesPrivate7

Done. Thanks:)

[cathiechen]

Change of this patch:
- Support ResizeObserver for Mac WK1. It won't invoke Page::willDisplayPage in WK1. Invoke checkResizeObservations before _flushCompositingChanges(). Or maybe we could call Page::willDisplayPage instead?
- Fixed WK1 layout test flush failures. In the layout test, it might not flush after layout. Schedule ResizeObserver timer after layout().
- Add back ENABLE(RESIZE_OBSERVER). ENABLE(RESIZE_OBSERVER) of win platform has been removed.

[Frédéric Wang (:fredw)]

@Ali, @Don: I talked with Cathie and the wincairo failure seems very similar to what happened in https://bugs.webkit.org/show_bug.cgi?id=179385#c24 ; maybe someone will need to force a clean rebuild when we land this?

[Fujii Hironori]

This WinCairo issue is filed in Bug 183177. We don't need force clean build. The next incremental build will pass. Thank you for letting us know before landing the patch. Feel free to land.

[Frédéric Wang (:fredw)]

Comment on attachment 366073 [details]
Patch

(In reply to Fujii Hironori from comment #120)
> This WinCairo issue is filed in Bug 183177. We don't need force clean build.
> The next incremental build will pass. Thank you for letting us know before
> landing the patch. Feel free to land.

Thanks!

[cathiechen]

*** Bug 193965 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 366073 [details]
Patch

Clearing flags on attachment: 366073

Committed r243643: <https://trac.webkit.org/changeset/243643>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like the test imported/w3c/web-platform-tests/resize-observer/eventloop.html

modified in https://trac.webkit.org/changeset/243643/webkit

is failing now. History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fresize-observer%2Feventloop.html

Diff:
--- /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/resize-observer/eventloop-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/resize-observer/eventloop-actual.txt
@@ -1,11 +1,12 @@
+CONSOLE MESSAGE: ResizeObserver loop completed with undelivered notifications.
+CONSOLE MESSAGE: ResizeObserver loop completed with undelivered notifications.
 CONSOLE MESSAGE: ResizeObserver loop completed with undelivered notifications.
 ResizeObserver notification event loop tests
 
-t1
-
-Harness Error (TIMEOUT), message = null
 
 PASS ResizeObserver implemented 
-NOTRUN guard 
-FAIL test0: multiple notifications inside same event loop assert_equals: new loop expected 1 but got 0
+PASS guard 
+PASS test0: multiple notifications inside same event loop 
+PASS test1: depths of shadow roots 
+PASS test2: move target in dom while inside event loop 
 


This looks like a rebaseline was needed. can you confirm?

[cathiechen]

(In reply to Truitt Savell from comment #125)
> It looks like the test
> imported/w3c/web-platform-tests/resize-observer/eventloop.html
> 
> modified in https://trac.webkit.org/changeset/243643/webkit
> 
> is failing now. History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fresize-
> observer%2Feventloop.html
> 
> Diff:
> ---
> /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/
> imported/w3c/web-platform-tests/resize-observer/eventloop-expected.txt
> +++
> /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/
> imported/w3c/web-platform-tests/resize-observer/eventloop-actual.txt
> @@ -1,11 +1,12 @@
> +CONSOLE MESSAGE: ResizeObserver loop completed with undelivered
> notifications.
> +CONSOLE MESSAGE: ResizeObserver loop completed with undelivered
> notifications.
>  CONSOLE MESSAGE: ResizeObserver loop completed with undelivered
> notifications.
>  ResizeObserver notification event loop tests
>  
> -t1
> -
> -Harness Error (TIMEOUT), message = null
>  
>  PASS ResizeObserver implemented 
> -NOTRUN guard 
> -FAIL test0: multiple notifications inside same event loop assert_equals:
> new loop expected 1 but got 0
> +PASS guard 
> +PASS test0: multiple notifications inside same event loop 
> +PASS test1: depths of shadow roots 
> +PASS test2: move target in dom while inside event loop 
>  
> 
> 
> This looks like a rebaseline was needed. can you confirm?

Hi Truitt,

This is affect by the order of notifyResizeObservers and requestAnimationFrame. They are not synchronized at current. I've filed bug https://bugs.webkit.org/show_bug.cgi?id=196422 to make it flaky.

[Ryosuke Niwa]

Comment on attachment 366073 [details]
Patch

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

> Source/WebCore/page/ResizeObserverEntry.idl:33
> +    readonly attribute Element target;

This has a GC issue. The wrapper of this target element can be erroneously collected.

[cathiechen]

Hi Ryosuke,

Sorry for the slow reply.


(In reply to Ryosuke Niwa from comment #127)
> Comment on attachment 366073 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366073&action=review
> 
> > Source/WebCore/page/ResizeObserverEntry.idl:33
> > +    readonly attribute Element target;
> 
> This has a GC issue. The wrapper of this target element can be erroneously
> collected.

Did you refer to https://bugs.webkit.org/show_bug.cgi?id=184307?

I did some research about this recently. But I couldn't reproduce the GC issues somehow.

- Target destruct before ResizeObserverEntry generated:
disconnectFromResizeObservers() in Element::~Element() would make sure delete target when it's destructed. So ResizeObserverEntry wouldn't be generated if the target is destructed.
- Target destruct after ResizeObserverEntry generated:
After ResizeObserverEntry generated in ResizeObserver::deliverObservations, it will invoke js callback immediately. So there is a jsobject(entries) hold it. I wrote a test case for it:

```
<script src="/resources/js-test-pre.js"></script>

<div style="width:100px; height:100px; margin:10px; border:1px solid black;" id="target1"></div>

<script>
var ro = new ResizeObserver( entries => {
  let target1 = document.querySelector('#target1');
  target1.parentElement.removeChild(target1);
  target1 = null;

  gc();

  for (let entry of entries) {
    const cr = entry.contentRect;
    var text = 'Element:' + entry.target + '\n';
    text = text + `Element size: ${cr.width}px x ${cr.height}px` + '\n';
    text = text + `Element padding: ${cr.top}px ; ${cr.left}px`;
    console.log(text);
  }
});

ro.observe(document.querySelector('#target1'));
  
</script>

```

Please let me know if I missed something. Thanks:)

[Shawn Roberts]

Layout test resize-observer/observe-element-from-other-frame.html added in https://trac.webkit.org/changeset/243643/webkit is a flaky failure on Mac Debug builds.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=resize-observer%2Fobserve-element-from-other-frame.html

Diff:

--- /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/resize-observer/observe-element-from-other-frame-expected.txt
+++ /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/resize-observer/observe-element-from-other-frame-actual.txt
@@ -1,5 +1,5 @@
 
 
 PASS ResizeObserver implemented 
-PASS test0: Observe element from other frame 
+FAIL test0: Observe element from other frame assert_equals: expected "success" but got "fail"

[cathiechen]

Re-opening for pull request https://github.com/WebKit/WebKit/pull/8839

[Simon Fraser (smfr)]

It would be better to use a new bug, rather than re-opening this one.

[cathiechen]

(In reply to Simon Fraser (smfr) from comment #131)
> It would be better to use a new bug, rather than re-opening this one.

Ah, sorry, I was trying to fix another bug(https://bugs.webkit.org/show_bug.cgi?id=250836), but I pasted this URL by mistake while creating the branch in github...