Bug 157743 - Implement ResizeObserver
Summary: Implement ResizeObserver
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords: InRadar, WebExposed
Depends on: 193965 193821
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-16 11:14 PDT by Simon Fraser (smfr)
Modified: 2019-02-19 19:40 PST (History)
21 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

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