|
Description
Simon Fraser (smfr)
2016-05-16 11:14:39 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. 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! 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. Would love to see this feature. Probably should be mentioned on https://webkit.org/status/ 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:) Created attachment 359876 [details]
Data structure and management
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 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. 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 (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. > 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.
(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 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! Created attachment 360287 [details]
WIP-002
Created attachment 360290 [details]
WIP-003
Created attachment 360291 [details]
WIP-004
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 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. 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 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. 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.
(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:) Created attachment 361897 [details]
Supported svg element
Passed svg.html
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? Created attachment 361903 [details]
Patch contains all changes
Sorry, the previous patch didn't include all changes.
> 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 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 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 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 Created attachment 361999 [details]
Fixed the review issues
Fixed the review issues.
Created attachment 362000 [details]
Fixed the review issues
Fixed the orders in construct.
Created attachment 362001 [details]
Change LayoutSize(0, 0) to LayoutSize()
Change LayoutSize(0, 0) to LayoutSize()
> >> 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 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 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 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 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 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 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.
Created attachment 363219 [details]
m_needsCheckResizeObservations
Add m_needsCheckResizeObservations to reduce notifyResizeObservers().
Created attachment 363505 [details]
Patch
Created attachment 363506 [details]
Patch
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? Created attachment 365042 [details]
Patch
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 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 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 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 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
Created attachment 365112 [details]
Support multi frames
- Support multi frames.
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 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 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 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 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 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 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 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
Created attachment 365144 [details]
troggle ENABLE(RESIZE_OBSERVER)
Created attachment 365145 [details]
Toggle ENABLE(RESIZE_OBSERVER) and fix ios build issue
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 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 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 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 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 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
Created attachment 365171 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error
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 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 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 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 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 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
Created attachment 365285 [details]
ENABLE_RESIZE_OBSERVER_and_ios_build_error
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 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 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 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 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 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 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. 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. :)
Please keep ENABLE_INTERSECTION_OBSERVER (sorry!). Not all ports may want to compile this in. > 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.
Created attachment 365479 [details]
Patch
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 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 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 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
Created attachment 365494 [details]
Patch
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 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 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 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 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 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
Created attachment 365702 [details]
Patch
- Add test case resize-observer/observe-element-from-other-frame.html - Deal with EWS WK1 error. Created attachment 365711 [details]
Patch
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 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 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 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 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 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
Created attachment 365953 [details]
Patch
Created attachment 365954 [details]
Patch
Created attachment 365969 [details]
Patch
Created attachment 365972 [details]
Patch
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 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
Created attachment 366064 [details]
Patch
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 Created attachment 366073 [details]
Patch
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:) 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. @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? 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 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! *** Bug 193965 has been marked as a duplicate of this bug. *** Comment on attachment 366073 [details] Patch Clearing flags on attachment: 366073 Committed r243643: <https://trac.webkit.org/changeset/243643> All reviewed patches have been landed. Closing bug. 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? (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 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. 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:) 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" |