Bug 157743 - Implement ResizeObserver
Summary: Implement ResizeObserver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL: https://w3c-test.org/resize-observer/
Keywords: InRadar, WebExposed
: 193965 (view as bug list)
Depends on: 193821 197457
Blocks: 196422
  Show dependency treegraph
 
Reported: 2016-05-16 11:14 PDT by Simon Fraser (smfr)
Modified: 2019-05-17 02:01 PDT (History)
28 users (show)

See Also:


Attachments
Data structure and management (31.94 KB, patch)
2019-01-23 06:55 PST, cathiechen
no flags Details | Formatted Diff | Diff
WIP-002 (80.64 KB, patch)
2019-01-27 02:55 PST, cathiechen
no flags Details | Formatted Diff | Diff
WIP-003 (81.02 KB, patch)
2019-01-27 04:07 PST, cathiechen
no flags Details | Formatted Diff | Diff
WIP-004 (80.91 KB, patch)
2019-01-27 04:13 PST, cathiechen
no flags Details | Formatted Diff | Diff
WIP-with-mainly-implement-except-svg-support (109.50 KB, patch)
2019-02-12 08:47 PST, cathiechen
no flags Details | Formatted Diff | Diff
Fixed the compile issue after rebase (110.54 KB, patch)
2019-02-12 10:36 PST, cathiechen
no flags Details | Formatted Diff | Diff
Supported svg element (62.17 KB, patch)
2019-02-12 22:37 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch contains all changes (105.89 KB, patch)
2019-02-13 00:51 PST, cathiechen
no flags Details | Formatted Diff | Diff
Fixed the review issues (105.69 KB, patch)
2019-02-13 23:51 PST, cathiechen
no flags Details | Formatted Diff | Diff
Fixed the review issues (105.69 KB, patch)
2019-02-14 00:06 PST, cathiechen
no flags Details | Formatted Diff | Diff
Change LayoutSize(0, 0) to LayoutSize() (105.68 KB, patch)
2019-02-14 00:18 PST, cathiechen
simon.fraser: review-
Details | Formatted Diff | Diff
Fixed the review issues (111.93 KB, patch)
2019-02-27 05:00 PST, cathiechen
no flags Details | Formatted Diff | Diff
m_needsCheckResizeObservations (113.19 KB, patch)
2019-02-28 05:06 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (129.84 KB, patch)
2019-03-04 05:55 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (129.88 KB, patch)
2019-03-04 06:26 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (156.09 KB, patch)
2019-03-18 12:08 PDT, cathiechen
simon.fraser: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.90 MB, application/zip)
2019-03-18 13:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (2.41 MB, application/zip)
2019-03-18 14:15 PDT, Build Bot
no flags Details
Support multi frames (150.04 KB, patch)
2019-03-18 19:46 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.80 MB, application/zip)
2019-03-18 21:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (2.43 MB, application/zip)
2019-03-18 23:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.97 MB, application/zip)
2019-03-19 00:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews201 for win-future (12.90 MB, application/zip)
2019-03-19 00:40 PDT, Build Bot
no flags Details
troggle ENABLE(RESIZE_OBSERVER) (150.34 KB, patch)
2019-03-19 03:07 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Toggle ENABLE(RESIZE_OBSERVER) and fix ios build issue (150.34 KB, patch)
2019-03-19 03:08 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.49 MB, application/zip)
2019-03-19 04:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.29 MB, application/zip)
2019-03-19 05:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews206 for win-future (12.91 MB, application/zip)
2019-03-19 05:50 PDT, Build Bot
no flags Details
ENABLE_RESIZE_OBSERVER_and_ios_build_error (150.04 KB, patch)
2019-03-19 09:33 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.49 MB, application/zip)
2019-03-19 10:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (2.39 MB, application/zip)
2019-03-19 11:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews205 for win-future (12.87 MB, application/zip)
2019-03-19 12:42 PDT, Build Bot
no flags Details
ENABLE_RESIZE_OBSERVER_and_ios_build_error (150.87 KB, patch)
2019-03-19 18:56 PDT, cathiechen
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.50 MB, application/zip)
2019-03-19 20:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews204 for win-future (12.87 MB, application/zip)
2019-03-19 22:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (2.48 MB, application/zip)
2019-03-19 22:59 PDT, Build Bot
no flags Details
Fixed code review issues and remove ENABLE_INTERSECTION_OBSERVER (103.51 KB, patch)
2019-03-20 09:27 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (107.89 KB, patch)
2019-03-20 21:17 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.49 MB, application/zip)
2019-03-20 22:32 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (2.31 MB, application/zip)
2019-03-20 23:16 PDT, Build Bot
no flags Details
Patch (107.89 KB, patch)
2019-03-20 23:29 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.77 MB, application/zip)
2019-03-21 00:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.44 MB, application/zip)
2019-03-21 01:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews205 for win-future (12.89 MB, application/zip)
2019-03-21 02:22 PDT, Build Bot
no flags Details
Patch (160.82 KB, patch)
2019-03-22 03:35 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (160.82 KB, patch)
2019-03-22 04:50 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.64 MB, application/zip)
2019-03-22 06:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.97 MB, application/zip)
2019-03-22 06:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (3.42 MB, application/zip)
2019-03-22 09:54 PDT, Build Bot
no flags Details
Patch (162.29 KB, patch)
2019-03-26 02:51 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (161.92 KB, patch)
2019-03-26 03:15 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (161.97 KB, patch)
2019-03-26 09:14 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (161.81 KB, patch)
2019-03-26 10:05 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.66 MB, application/zip)
2019-03-26 15:33 PDT, Build Bot
no flags Details
Patch (162.13 KB, patch)
2019-03-27 01:30 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (161.57 KB, patch)
2019-03-27 08:41 PDT, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2016-05-16 11:14:39 PDT
https://wicg.github.io/ResizeObserver/
Comment 1 Radar WebKit Bug Importer 2016-05-16 11:16:50 PDT
<rdar://problem/26302223>
Comment 2 Philip Jägenstedt 2017-10-13 05:04:48 PDT
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.
Comment 3 William J. Edney 2017-11-22 11:23:24 PST
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!
Comment 4 martin 2018-08-29 22:25:14 PDT
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.
Comment 5 Eric Gorr 2019-01-22 12:07:39 PST
Would love to see this feature.
Comment 6 Eric Gorr 2019-01-22 12:07:57 PST
Probably should be mentioned on https://webkit.org/status/
Comment 7 cathiechen 2019-01-23 02:09:12 PST
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:)
Comment 8 cathiechen 2019-01-23 06:55:54 PST
Created attachment 359876 [details]
Data structure and management
Comment 9 Ali Juma 2019-01-23 07:41:30 PST
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).
Comment 10 Ali Juma 2019-01-23 07:48:04 PST
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.
Comment 11 Aleksandar Totic 2019-01-23 08:48:03 PST
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
Comment 12 Simon Fraser (smfr) 2019-01-23 09:21:16 PST
(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.
Comment 13 Aleksandar Totic 2019-01-23 15:14:49 PST
> 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.
Comment 14 Ryosuke Niwa 2019-01-23 16:10:40 PST
(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.
Comment 15 cathiechen 2019-01-27 02:54:09 PST
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!
Comment 16 cathiechen 2019-01-27 02:55:04 PST
Created attachment 360287 [details]
WIP-002
Comment 17 cathiechen 2019-01-27 04:07:22 PST
Created attachment 360290 [details]
WIP-003
Comment 18 cathiechen 2019-01-27 04:13:28 PST
Created attachment 360291 [details]
WIP-004
Comment 19 Frédéric Wang (:fredw) 2019-02-06 08:29:53 PST
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:
Comment 20 cathiechen 2019-02-10 23:36:57 PST
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.
Comment 21 cathiechen 2019-02-12 08:47:18 PST
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
Comment 22 Rob Buis 2019-02-12 09:10:56 PST
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.
Comment 23 cathiechen 2019-02-12 10:36:30 PST
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.
Comment 24 cathiechen 2019-02-12 10:38:40 PST
(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:)
Comment 25 cathiechen 2019-02-12 22:37:10 PST
Created attachment 361897 [details]
Supported svg element

Passed svg.html
Comment 26 Rob Buis 2019-02-12 23:45:13 PST
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
> +2019-02-12  cathie chen  <cathiechen@igalia.com>

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?
Comment 27 cathiechen 2019-02-13 00:51:34 PST
Created attachment 361903 [details]
Patch contains all changes

Sorry, the previous patch didn't include all changes.
Comment 28 cathiechen 2019-02-13 01:00:06 PST
> 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
> > +2019-02-12  cathie chen  <cathiechen@igalia.com>
> 
> 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.
Comment 29 Rob Buis 2019-02-13 01:18:26 PST
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.
Comment 30 Rob Buis 2019-02-13 05:42:46 PST
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.
Comment 31 cathiechen 2019-02-13 23:48:55 PST
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
Comment 32 cathiechen 2019-02-13 23:51:46 PST
Created attachment 361999 [details]
Fixed the review issues

Fixed the review issues.
Comment 33 cathiechen 2019-02-14 00:06:05 PST
Created attachment 362000 [details]
Fixed the review issues

Fixed the orders in construct.
Comment 34 cathiechen 2019-02-14 00:18:38 PST
Created attachment 362001 [details]
Change LayoutSize(0, 0) to LayoutSize()

Change LayoutSize(0, 0) to LayoutSize()
Comment 35 cathiechen 2019-02-14 00:19:57 PST
> >> 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()
Comment 36 Rob Buis 2019-02-14 01:24:36 PST
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.
Comment 37 zalan 2019-02-14 10:36:44 PST
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)
Comment 38 Simon Fraser (smfr) 2019-02-14 10:37:54 PST
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 ?
Comment 39 cathiechen 2019-02-19 19:40:50 PST
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
Comment 40 cathiechen 2019-02-27 04:51:06 PST
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
Comment 41 cathiechen 2019-02-27 05:00:53 PST
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.
Comment 42 cathiechen 2019-02-28 05:06:31 PST
Created attachment 363219 [details]
m_needsCheckResizeObservations

Add m_needsCheckResizeObservations to reduce notifyResizeObservers().
Comment 43 cathiechen 2019-03-04 05:55:56 PST
Created attachment 363505 [details]
Patch
Comment 44 cathiechen 2019-03-04 06:26:46 PST
Created attachment 363506 [details]
Patch
Comment 45 Simon Fraser (smfr) 2019-03-04 11:37:02 PST
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?
Comment 46 cathiechen 2019-03-18 12:08:13 PDT
Created attachment 365042 [details]
Patch
Comment 47 Build Bot 2019-03-18 13:27:28 PDT
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
Comment 48 Build Bot 2019-03-18 13:27:30 PDT
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
Comment 49 Simon Fraser (smfr) 2019-03-18 14:04:41 PDT
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
Comment 50 Build Bot 2019-03-18 14:15:40 PDT
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
Comment 51 Build Bot 2019-03-18 14:15:43 PDT
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
Comment 52 cathiechen 2019-03-18 19:46:53 PDT
Created attachment 365112 [details]
Support multi frames

- Support multi frames.
Comment 53 Build Bot 2019-03-18 21:05:32 PDT
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
Comment 54 Build Bot 2019-03-18 21:05:35 PDT
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
Comment 55 Build Bot 2019-03-18 23:15:59 PDT
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
Comment 56 Build Bot 2019-03-18 23:16:02 PDT
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
Comment 57 Build Bot 2019-03-19 00:01:07 PDT
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
Comment 58 Build Bot 2019-03-19 00:01:09 PDT
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
Comment 59 Build Bot 2019-03-19 00:40:42 PDT
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
Comment 60 Build Bot 2019-03-19 00:40:55 PDT
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
Comment 61 cathiechen 2019-03-19 03:07:47 PDT
Created attachment 365144 [details]
troggle ENABLE(RESIZE_OBSERVER)
Comment 62 cathiechen 2019-03-19 03:08:52 PDT
Created attachment 365145 [details]
Toggle ENABLE(RESIZE_OBSERVER) and fix ios build issue
Comment 63 Build Bot 2019-03-19 04:30:29 PDT
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
Comment 64 Build Bot 2019-03-19 04:30:31 PDT
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
Comment 65 Build Bot 2019-03-19 05:11:50 PDT
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
Comment 66 Build Bot 2019-03-19 05:11:52 PDT
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
Comment 67 Build Bot 2019-03-19 05:50:32 PDT
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
Comment 68 Build Bot 2019-03-19 05:50:44 PDT
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
Comment 69 cathiechen 2019-03-19 09:33:20 PDT
Created attachment 365171 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error
Comment 70 Build Bot 2019-03-19 10:55:58 PDT
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
Comment 71 Build Bot 2019-03-19 10:56:01 PDT
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
Comment 72 Build Bot 2019-03-19 11:41:49 PDT
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
Comment 73 Build Bot 2019-03-19 11:41:52 PDT
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
Comment 74 Build Bot 2019-03-19 12:41:49 PDT
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
Comment 75 Build Bot 2019-03-19 12:42:02 PDT
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
Comment 76 cathiechen 2019-03-19 18:56:10 PDT
Created attachment 365285 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error
Comment 77 Build Bot 2019-03-19 20:56:13 PDT
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
Comment 78 Build Bot 2019-03-19 20:56:16 PDT
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
Comment 79 Build Bot 2019-03-19 22:43:20 PDT
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
Comment 80 Build Bot 2019-03-19 22:43:32 PDT
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
Comment 81 Build Bot 2019-03-19 22:59:17 PDT
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
Comment 82 Build Bot 2019-03-19 22:59:19 PDT
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
Comment 83 cathiechen 2019-03-20 01:25:37 PDT
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.
Comment 84 cathiechen 2019-03-20 09:27:09 PDT
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. :)
Comment 85 Simon Fraser (smfr) 2019-03-20 09:57:49 PDT
Please keep ENABLE_INTERSECTION_OBSERVER (sorry!). Not all ports may want to compile this in.
Comment 86 cathiechen 2019-03-20 21:14:01 PDT
> 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.
Comment 87 cathiechen 2019-03-20 21:17:50 PDT
Created attachment 365479 [details]
Patch
Comment 88 Build Bot 2019-03-20 22:32:27 PDT
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
Comment 89 Build Bot 2019-03-20 22:32:30 PDT
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
Comment 90 Build Bot 2019-03-20 23:16:37 PDT
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
Comment 91 Build Bot 2019-03-20 23:16:39 PDT
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
Comment 92 cathiechen 2019-03-20 23:29:14 PDT
Created attachment 365494 [details]
Patch
Comment 93 Build Bot 2019-03-21 00:33:15 PDT
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
Comment 94 Build Bot 2019-03-21 00:33:17 PDT
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
Comment 95 Build Bot 2019-03-21 01:25:18 PDT
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
Comment 96 Build Bot 2019-03-21 01:25:21 PDT
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
Comment 97 Build Bot 2019-03-21 02:22:31 PDT
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
Comment 98 Build Bot 2019-03-21 02:22:43 PDT
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
Comment 99 cathiechen 2019-03-22 03:35:34 PDT
Created attachment 365702 [details]
Patch
Comment 100 cathiechen 2019-03-22 03:38:56 PDT
- Add test case resize-observer/observe-element-from-other-frame.html
- Deal with EWS WK1 error.
Comment 101 cathiechen 2019-03-22 04:50:17 PDT
Created attachment 365711 [details]
Patch
Comment 102 Build Bot 2019-03-22 06:15:54 PDT
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
Comment 103 Build Bot 2019-03-22 06:15:57 PDT
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
Comment 104 Build Bot 2019-03-22 06:37:11 PDT
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
Comment 105 Build Bot 2019-03-22 06:37:14 PDT
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
Comment 106 Build Bot 2019-03-22 09:54:48 PDT
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
Comment 107 Build Bot 2019-03-22 09:54:52 PDT
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
Comment 108 cathiechen 2019-03-26 02:51:03 PDT
Created attachment 365953 [details]
Patch
Comment 109 cathiechen 2019-03-26 03:15:55 PDT
Created attachment 365954 [details]
Patch
Comment 110 cathiechen 2019-03-26 09:14:09 PDT
Created attachment 365969 [details]
Patch
Comment 111 cathiechen 2019-03-26 10:05:01 PDT
Created attachment 365972 [details]
Patch
Comment 112 Build Bot 2019-03-26 15:33:18 PDT
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
Comment 113 Build Bot 2019-03-26 15:33:21 PDT
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
Comment 114 cathiechen 2019-03-27 01:30:22 PDT
Created attachment 366064 [details]
Patch
Comment 115 Frédéric Wang (:fredw) 2019-03-27 08:14:33 PDT
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
Comment 116 cathiechen 2019-03-27 08:41:16 PDT
Created attachment 366073 [details]
Patch
Comment 117 cathiechen 2019-03-27 09:49:31 PDT
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:)
Comment 118 cathiechen 2019-03-27 09:53:46 PDT
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.
Comment 119 Frédéric Wang (:fredw) 2019-03-29 00:44:51 PDT
@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?
Comment 120 Fujii Hironori 2019-03-29 00:54:01 PDT
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.
Comment 121 Frédéric Wang (:fredw) 2019-03-29 01:09:41 PDT
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!
Comment 122 cathiechen 2019-03-29 01:22:11 PDT
*** Bug 193965 has been marked as a duplicate of this bug. ***
Comment 123 WebKit Commit Bot 2019-03-29 01:37:52 PDT
Comment on attachment 366073 [details]
Patch

Clearing flags on attachment: 366073

Committed r243643: <https://trac.webkit.org/changeset/243643>
Comment 124 WebKit Commit Bot 2019-03-29 01:37:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 125 Truitt Savell 2019-03-29 11:18:36 PDT
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?
Comment 126 cathiechen 2019-03-29 21:35:56 PDT
(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.
Comment 127 Ryosuke Niwa 2019-04-01 11:28:02 PDT
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.
Comment 128 cathiechen 2019-04-08 02:12:24 PDT
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:)
Comment 129 Shawn Roberts 2019-04-18 10:40:16 PDT
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"