Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2005593002:
Initial ResizeObserver implementation (Closed)
|
Created:
4 years, 7 months ago by atotic1 Modified:
4 years, 3 months ago CC:
chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, kinuko+watch, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis is initial ResizeObserver implementation of the spec https://github.com/WICG/ResizeObserver/, with test cases.
This CR does not completely implement the spec. The differences will be resolved in upcoming CRs. The differences are:
1) Notification loop breaks on numeric limit. Should use depth limit.
2) Notification loop polls for size changes. Push is more performant.
3) ResizeObserverEntry should have DOMRectReadOnly, not clientRect.
DOMRectReadOnly is still behind a flag, will fix when it lands.
BUG=612962
Patch Set 1 #
Total comments: 87
Patch Set 2 : CR fixes. #Patch Set 3 : Observe content box, not clientWidth #
Total comments: 36
Patch Set 4 : CR fixes, + test suite #Patch Set 5 : Update to master: Vector<WeakMember> no longer ok. #
Total comments: 23
Patch Set 6 : CR fixes + update to master #Patch Set 7 : Fix global-interface-listing test #
Total comments: 10
Messages
Total messages: 33 (13 generated)
Description was changed from ========== First stab at ResizeObserver. Oustanding problems: ResizeObserver::m_observations links should be weak we should watch LayoutUnit clientWidth, instead of int clientWidht FrameView::notifyResizeObserver loop needs review by someone who knows eventloop BUG=612962 ========== to ========== First stab at ResizeObserver. Oustanding problems: ResizeObserver::m_observations links should be weak we should watch LayoutUnit clientWidth, instead of int clientWidht FrameView::notifyResizeObserver loop needs review by someone who knows eventloop BUG=612962 ==========
atotic@google.com changed reviewers: + szager@chromium.org
Description was changed from ========== First stab at ResizeObserver. Oustanding problems: ResizeObserver::m_observations links should be weak we should watch LayoutUnit clientWidth, instead of int clientWidht FrameView::notifyResizeObserver loop needs review by someone who knows eventloop BUG=612962 ========== to ========== First stab at ResizeObserver. Oustanding problems: "What to watch" is still in flux. clientWidth has problems. FrameView::notifyResizeObserver loop needs review by someone who knows eventloop BUG=612962 ==========
https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.h:693: bool hasResizeObserverController() const { return m_resizeObserverController; } This should just be: ResizeObserverController* resizeObserverController() const { return m_resizeObserverController; } This is the preferred way to bool-check a Member<>. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.h:1396: Member<ResizeObserverController> Don't wrap. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ElementRareData.h:38: #include "core/observer/ResizeObservation.h" Move this to ElementRareData.cpp, and add a forward class declaration to this file. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:29: #include "bindings/core/v8/ScriptCallStack.h" I don't think you use this. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2433: while (!done) { I think this loop might read better as: do { resizeController.deliverObservations(); if (state >= LayoutClean && !needsLayout()) break; updateStyleAndLayoutIfNeededRecursive(); if (++resizeNotifyCount > limit) { generate error; break; } resizeObserver.gatherObservations(); } while (resizeController.hasObservations()); ASSERT(!layoutView()->needsLayout()); Advantages: - It clarifies that the exit condition for the loop is when there are no more observations. - The document is in a consistent state when the loop exits (hence the ASSERT). https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2455: if (resizeNotifyCount == ResizeObserverController::kRenderLoopLimit) { If you're going to generate an error, you should do it *before* gather/deliver, and early-out. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2486: notifyResizeObserver(); Maybe rename this to runUpdateNotifyLoop(), and remove the updateStyleAndLayoutIfNeededRecursive() call just before this. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObservation.cpp (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.cpp:11: ResizeObservation::ResizeObservation(Element * target, ResizeObserver * observer) : Colon goes on next line; refer to existing class declarations. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.cpp:12: m_target(target), Comma goes at the beginning of the next line, for all constructors. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObservation.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:8: #include "bindings/core/v8/ScriptWrappable.h" I don't think you use this. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:9: #include "core/dom/Element.h" You shouldn't need this, the forward class declaration is sufficient. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:19: class ResizeObservation : public GarbageCollectedFinalized<ResizeObservation> { Too many blank lines. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:23: ResizeObservation(Element * target, ResizeObserver *); No space between "Element" and "*". Here and everywhere in this patch. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:25: virtual ~ResizeObservation() {}; No spurious destructors: get rid of this and change class declaration to us GarbageCollected<ResizeObservation>. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:30: Element * target() const { return m_target; } No space between "Element" and "*" https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:43: Member<ResizeObserver> m_observer; // Used for GC only. I think this comment is confusing; it doesn't give enough information to really be useful. I suggest getting rid of it. If you want to provide an explanation of gc semantics for this class (which I don't think is necessary, but feel free), then it should go in a README.md. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:44: int m_broadcastWidth; // Last broadcast clientWidth. We need to use LayoutUnit everywhere, not int. For javascript interfaces, convert to/from float. To be concise, use LayoutSize: LayoutSize broadcastSize() const { return m_broadcastSize; } void setBroadcastSize(LayoutSize size) { m_broadcastSize = size; } private: LayoutSize m_broadcastSize; https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:44: int m_broadcastWidth; // Last broadcast clientWidth. Instead of the comment, just rename the field to m_lastBroadcastSize. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:48: class ResizeObservationSet : public GarbageCollectedFinalized<ResizeObservationSet> { We don't really use custom collection wrapper classes in blink; just use HeapHashSet<Member<ResizeObservation>> everywhere. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserver.cpp (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:25: bool compareObservationToElement(Member<blink::ResizeObservation> observation, Element* el) This isn't used. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:28: } Add blank line. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:29: bool operator==(Member<blink::ResizeObservation> observation, Element* el) meh, this is obfuscating. Just use (observation->target() == el) where necessary. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:36: if (target && m_observations.find(target) == kNotFound) This is why, for IntersectionObserver, I did this: class NodeIntersectionObserverData { public: IntersectionObservation* getObservationFor(IntersectionObserver&); private: HeapHashMap<Member<IntersectionObserver>, Member<IntersectionObservation>> m_observations; } ... to enable this: void IntersectionObserver::observe(Element* target) { if (target->ensureIntersectionObserverData().getObservationFor(*this)) return; } The HashMap lookup will be faster than the list lookup you're doing here. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:50: m_activeObservations.clear(); Also: m_callback.clear(); https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:55: m_activeObservations.clear(); This should be either ASSERT(m_activeObservations->isEmpty()), or it should be nothing. Clearing out possible pending notifications can't be the desired behavior. Also, this would need to be m_activeObservations->clear(), not m_activeObservations.clear(). https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:57: for (auto observation : m_observations) { I'm not sure if it makes a difference, but maybe it should be "auto&" rather that "auto" ? I honestly don't know if it matters. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:70: for (size_t i = 0; i < m_activeObservations.size(); i++) { for (auto& observation : m_activeObservations) https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:76: m_callback->handleEvent(entries, this); m_activeObservations->clear(); https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserver.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.h:8: #include "bindings/core/v8/ScriptWrappable.h" I don't think you use this. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.h:9: #include "core/dom/Element.h" You shouldn't need this, the forward class declaration is sufficient. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.h:10: #include "core/observer/ResizeObserverEntry.h" I don't think you use this. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.h:18: class ResizeObserverEntry; I don't think you use this. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.h:29: virtual ~ResizeObserver() {}; No unnecessary destructors, switch class to GarbageCollected<> https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.h:38: void gatherObservations(); Too much whitespace. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.h:47: using ObservationList = HeapVector<Member<ResizeObservation>>; No typedef's or "using", just use full type everywhere. Also, should this be HeapVector<WeakMember<ResizeObservation>> ? If a ResizeObservation's target Element goes away, the ResizeObserver should not keep the ResizeObservation alive. And consider using HashSet rather than Vector, for fast lookup/remove. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserverController.cpp (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:23: for (auto it = m_observers.begin(); it != m_observers.end(); ++it) for (auto& observer : m_observers) observer->gatherObservations(); https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:29: for (auto it = m_observers.begin(); it != m_observers.end(); ++it) { This shouldn't do an iteration. Consider making ResizeObserver::gatherObservations() return a bool indicating whether any new notifications were generated, and then: void ResizeObserverController::gatherObservations { for (auto& observer : m_observers) m_hasNotifications |= observer->gatherObservations(); } void ResizeObserverController::deliverObservations() { for (auto& observer : m_observers) observer->deliverObservations(); m_hasNotifications = false; } https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:38: for (auto it = m_observers.begin(); it != m_observers.end(); ++it) ditto iterator syntax. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserverController.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.h:23: virtual ~ResizeObserverController() {}; Ditto comment about unnecessary destructors. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.h:26: Too many blank lines. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.h:38: HeapHashSet<Member<ResizeObserver>> m_observers; This should probably be HeapHashSet<WeakMember<ResizeObserver>>. The document should not keep the observers alive. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:25: virtual ~ResizeObserverEntry() {}; Unnecessary destructor. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:28: int clientWidth() const { return m_clientWidth; } s/int/float/ https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:35: int m_clientWidth; s/int/LayoutUnit/ https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.idl (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverEntry.idl:11: readonly attribute long clientWidth; s/long/double/ https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:176: ResizeObserver status=stable status=experimental
Description was changed from ========== First stab at ResizeObserver. Oustanding problems: "What to watch" is still in flux. clientWidth has problems. FrameView::notifyResizeObserver loop needs review by someone who knows eventloop BUG=612962 ========== to ========== First stab at ResizeObserver. Oustanding problems: "What to watch" is still in flux. clientWidth will go away, and all units will be floats. Current proposal: - watch content size - report padding size, content size FrameView::notifyResizeObserver loop needs review by someone who knows eventloop BUG=612962 ==========
This code should address all of the CR issues except: - clientWidth and related fields not being LayoutUnit This is not addressed because clientWidth is on its way out. What we observe and report will change. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.h:693: bool hasResizeObserverController() const { return m_resizeObserverController; } On 2016/05/23 19:49:10, szager1 wrote: > This should just be: > > ResizeObserverController* resizeObserverController() const { return > m_resizeObserverController; } > > This is the preferred way to bool-check a Member<>. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.h:1396: Member<ResizeObserverController> On 2016/05/23 19:49:10, szager1 wrote: > Don't wrap. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ElementRareData.h:38: #include "core/observer/ResizeObservation.h" On 2016/05/23 19:49:10, szager1 wrote: > Move this to ElementRareData.cpp, and add a forward class declaration to this > file. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:29: #include "bindings/core/v8/ScriptCallStack.h" On 2016/05/23 19:49:10, szager1 wrote: > I don't think you use this. I am using it, through magic of templates. Won't compile without it. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2433: while (!done) { On 2016/05/23 19:49:10, szager1 wrote: > I think this loop might read better as: > > do { > resizeController.deliverObservations(); > if (state >= LayoutClean && !needsLayout()) > break; > > updateStyleAndLayoutIfNeededRecursive(); > > if (++resizeNotifyCount > limit) { > generate error; > break; > } > > resizeObserver.gatherObservations(); > } while (resizeController.hasObservations()); > > ASSERT(!layoutView()->needsLayout()); > > > > Advantages: > > - It clarifies that the exit condition for the loop is when there are no more > observations. > - The document is in a consistent state when the loop exits (hence the ASSERT). I reworked the loop into something more understandeable. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2455: if (resizeNotifyCount == ResizeObserverController::kRenderLoopLimit) { On 2016/05/23 19:49:10, szager1 wrote: > If you're going to generate an error, you should do it *before* gather/deliver, > and early-out. Acknowledged. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:2486: notifyResizeObserver(); On 2016/05/23 19:49:10, szager1 wrote: > Maybe rename this to runUpdateNotifyLoop(), and I'd prefer not to rename it until it does something more than resize. updateNotify does not convey much information. > remove the updateStyleAndLayoutIfNeededRecursive() call just before this. If resizeObserver is undefined, it will not run the updateStyleAndLayoutIfNeededRecursive() Leaving it in for now. This whole code segment needs a review by someone who understands the event loop. I do not understand the event cycle/layout enough to understand interaction between lifecycle().state(), needsLayout(), and updateStyleAndLayoutIfNeededRecursive() https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObservation.cpp (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.cpp:11: ResizeObservation::ResizeObservation(Element * target, ResizeObserver * observer) : On 2016/05/23 19:49:10, szager1 wrote: > Colon goes on next line; refer to existing class declarations. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.cpp:12: m_target(target), On 2016/05/23 19:49:10, szager1 wrote: > Comma goes at the beginning of the next line, for all constructors. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObservation.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:8: #include "bindings/core/v8/ScriptWrappable.h" On 2016/05/23 19:49:10, szager1 wrote: > I don't think you use this. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:9: #include "core/dom/Element.h" On 2016/05/23 19:49:10, szager1 wrote: > You shouldn't need this, the forward class declaration is sufficient. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:19: class ResizeObservation : public GarbageCollectedFinalized<ResizeObservation> { On 2016/05/23 19:49:11, szager1 wrote: > Too many blank lines. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:23: ResizeObservation(Element * target, ResizeObserver *); On 2016/05/23 19:49:11, szager1 wrote: > No space between "Element" and "*". Here and everywhere in this patch. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:25: virtual ~ResizeObservation() {}; On 2016/05/23 19:49:11, szager1 wrote: > No spurious destructors: get rid of this and change class declaration to us > GarbageCollected<ResizeObservation>. My understanding was that unless class is GarbageCollectedFinalized, its destructor will not be called. This class does need a destructor, because it has a member whose destructor should be called: HeapHashSet<Member<ResizeObservation>> m_observations Who will be calling HeapHashSet's destructor? https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:30: Element * target() const { return m_target; } On 2016/05/23 19:49:11, szager1 wrote: > No space between "Element" and "*" Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:43: Member<ResizeObserver> m_observer; // Used for GC only. On 2016/05/23 19:49:10, szager1 wrote: > I think this comment is confusing; it doesn't give enough information to really > be useful. I suggest getting rid of it. If you want to provide an explanation > of gc semantics for this class (which I don't think is necessary, but feel > free), then it should go in a README.md. If I was refactoring a class, and saw a class member that was not used anywhere, I'd be tempted to get rid of it. This comment clarifies that in spite of being unused, the variable needs to stay. What's the harm in keeping in? https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:44: int m_broadcastWidth; // Last broadcast clientWidth. On 2016/05/23 19:49:11, szager1 wrote: > We need to use LayoutUnit everywhere, not int. For javascript interfaces, > convert to/from float. > > To be concise, use LayoutSize: > > LayoutSize broadcastSize() const { return m_broadcastSize; } > void setBroadcastSize(LayoutSize size) { m_broadcastSize = size; } > private: > LayoutSize m_broadcastSize; LayoutUnit vs int: current code returns an int, that's why I am holding onto an int. The size code is still in flux. Added a TODO, will take under advice in next rewrite. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:44: int m_broadcastWidth; // Last broadcast clientWidth. On 2016/05/23 19:49:11, szager1 wrote: > We need to use LayoutUnit everywhere, not int. For javascript interfaces, > convert to/from float. > > To be concise, use LayoutSize: > > LayoutSize broadcastSize() const { return m_broadcastSize; } > void setBroadcastSize(LayoutSize size) { m_broadcastSize = size; } > private: > LayoutSize m_broadcastSize; Acknowledged. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:48: class ResizeObservationSet : public GarbageCollectedFinalized<ResizeObservationSet> { On 2016/05/23 19:49:10, szager1 wrote: > We don't really use custom collection wrapper classes in blink; just use > HeapHashSet<Member<ResizeObservation>> everywhere. That's makes for some ugly looking declarations, and makes the collection type harder to change. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserver.cpp (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:25: bool compareObservationToElement(Member<blink::ResizeObservation> observation, Element* el) On 2016/05/23 19:49:11, szager1 wrote: > This isn't used. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:28: } On 2016/05/23 19:49:11, szager1 wrote: > Add blank line. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:29: bool operator==(Member<blink::ResizeObservation> observation, Element* el) On 2016/05/23 19:49:11, szager1 wrote: > meh, this is obfuscating. Just use (observation->target() == el) where > necessary. Leftover from when I was using the std::find algorithm. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:36: if (target && m_observations.find(target) == kNotFound) On 2016/05/23 19:49:11, szager1 wrote: > This is why, for IntersectionObserver, I did this: > > class NodeIntersectionObserverData { > public: > IntersectionObservation* getObservationFor(IntersectionObserver&); > private: > HeapHashMap<Member<IntersectionObserver>, Member<IntersectionObservation>> > m_observations; > } > > ... to enable this: > > void IntersectionObserver::observe(Element* target) > { > if (target->ensureIntersectionObserverData().getObservationFor(*this)) > return; > } > > The HashMap lookup will be faster than the list lookup you're doing here. Done. I was not optimizing this case because we traverse the entire list on every observation. As observe/unobserve are hopefully called much less often, having them perform the same traversal did not seem like a significant performance hit. This does introduce additional complexity, because knowledge of whether an Element has a ResizeObservation is held in two locations, that must be kept in sync. Note the additional complexity in unobserve() and disconnect() https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:50: m_activeObservations.clear(); On 2016/05/23 19:49:11, szager1 wrote: > Also: > > m_callback.clear(); That'd violate the spec, because no elements could be observed afterwards. I followed MutationObserver behavior, that allows observe() after disconnect https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:55: m_activeObservations.clear(); On 2016/05/23 19:49:11, szager1 wrote: > This should be either ASSERT(m_activeObservations->isEmpty()), or it should be > nothing. Clearing out possible pending notifications can't be the desired > behavior. > > Also, this would need to be m_activeObservations->clear(), not > m_activeObservations.clear(). ??? m_activeObservations->clear() does not compile. Clearing old observations is the desired behavior. We always test all observations, so there is no value in keeping the old ones. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:57: for (auto observation : m_observations) { On 2016/05/23 19:49:11, szager1 wrote: > I'm not sure if it makes a difference, but maybe it should be "auto&" rather > that "auto" ? I honestly don't know if it matters. auto& means you can't have a null. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:70: for (size_t i = 0; i < m_activeObservations.size(); i++) { On 2016/05/23 19:49:11, szager1 wrote: > for (auto& observation : m_activeObservations) Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:76: m_callback->handleEvent(entries, this); On 2016/05/23 19:49:11, szager1 wrote: > m_activeObservations->clear(); Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserverController.cpp (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:23: for (auto it = m_observers.begin(); it != m_observers.end(); ++it) On 2016/05/23 19:49:12, szager1 wrote: > for (auto& observer : m_observers) > observer->gatherObservations(); Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:29: for (auto it = m_observers.begin(); it != m_observers.end(); ++it) { On 2016/05/23 19:49:12, szager1 wrote: > This shouldn't do an iteration. Consider making > ResizeObserver::gatherObservations() return a bool indicating whether any new > notifications were generated, and then: > > void ResizeObserverController::gatherObservations > { > for (auto& observer : m_observers) > m_hasNotifications |= observer->gatherObservations(); > } > > void ResizeObserverController::deliverObservations() > { > for (auto& observer : m_observers) > observer->deliverObservations(); > m_hasNotifications = false; > } That was my original design, broke it apart to experiment with different algorithms. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:38: for (auto it = m_observers.begin(); it != m_observers.end(); ++it) On 2016/05/23 19:49:12, szager1 wrote: > ditto iterator syntax. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserverController.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.h:23: virtual ~ResizeObserverController() {}; On 2016/05/23 19:49:12, szager1 wrote: > Ditto comment about unnecessary destructors. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.h:26: On 2016/05/23 19:49:12, szager1 wrote: > Too many blank lines. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverController.h:38: HeapHashSet<Member<ResizeObserver>> m_observers; On 2016/05/23 19:49:12, szager1 wrote: > This should probably be HeapHashSet<WeakMember<ResizeObserver>>. The document > should not keep the observers alive. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:25: virtual ~ResizeObserverEntry() {}; On 2016/05/23 19:49:12, szager1 wrote: > Unnecessary destructor. Done. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:28: int clientWidth() const { return m_clientWidth; } On 2016/05/23 19:49:12, szager1 wrote: > s/int/float/ Will do when clientWidth gets rewritten. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:35: int m_clientWidth; On 2016/05/23 19:49:12, szager1 wrote: > s/int/LayoutUnit/ Will do when clientWidth gets rewritten. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.idl (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserverEntry.idl:11: readonly attribute long clientWidth; On 2016/05/23 19:49:12, szager1 wrote: > s/long/double/ clientWidth will change. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:176: ResizeObserver status=stable On 2016/05/23 19:49:12, szager1 wrote: > status=experimental Done.
Description was changed from ========== First stab at ResizeObserver. Oustanding problems: "What to watch" is still in flux. clientWidth will go away, and all units will be floats. Current proposal: - watch content size - report padding size, content size FrameView::notifyResizeObserver loop needs review by someone who knows eventloop BUG=612962 ========== to ========== First stab at ResizeObserver. It mostly works, with few outstanding issues: - Notification loop breaks on numeric limit. Should use depth limit. - Notification loop polls for size changes. Ojan would like push. - ResizeObserverEntry should have DOMReadOnlyRect, not clientRect. DOMReadOnlyRect is still behind a flag FrameView::notifyResizeObserver loop needs review by someone who understands eventloop BUG=612962 ==========
This commit addresses most CR issues. Big CR related changes were: - created a HashMap on ElementRareData between Observer and Observation - reworked the event loop code for clarity There are still issues that need clarification: https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... New feature: clientWidth and clientHeight are no longer observed. We are observing content box instead.
https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObservation.h (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObservation.h:25: virtual ~ResizeObservation() {}; On 2016/05/25 00:15:52, atotic1 wrote: > On 2016/05/23 19:49:11, szager1 wrote: > > No spurious destructors: get rid of this and change class declaration to us > > GarbageCollected<ResizeObservation>. > > My understanding was that unless class is GarbageCollectedFinalized, its > destructor will not be called. > > This class does need a destructor, because it has a member whose destructor > should be called: > HeapHashSet<Member<ResizeObservation>> m_observations > Who will be calling HeapHashSet's destructor? HeapHashSet does not have a destructor. https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/observer/ResizeObserver.cpp (right): https://codereview.chromium.org/2005593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:55: m_activeObservations.clear(); On 2016/05/25 00:15:53, atotic1 wrote: > On 2016/05/23 19:49:11, szager1 wrote: > > This should be either ASSERT(m_activeObservations->isEmpty()), or it should be > > nothing. Clearing out possible pending notifications can't be the desired > > behavior. > > > > Also, this would need to be m_activeObservations->clear(), not > > m_activeObservations.clear(). > > ??? m_activeObservations->clear() does not compile. > > Clearing old observations is the desired behavior. > We always test all observations, so there is no value in keeping the old ones. It should never happen that gatherObservations() is called when there are still un-delivered observations. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:693: bool resizeObserverController() const { return m_resizeObserverController; } I would still prefer that you honor existing convention, like these: StyleResolver* styleResolver() const; StyleResolver& ensureStyleResolver() const; IntersectionObserverController* intersectionObserverController(); IntersectionObserverController& ensureIntersectionObserverController(); Document& ensureTemplateDocument(); Document* templateDocumentHost() { return m_templateDocumentHost; } EventTargetData* eventTargetData() override; EventTargetData& ensureEventTargetData() override; NodeRareData* rareData() const; NodeRareData& ensureRareData(); Some background on why we discourage implicit bool conversions from concrete types: http://www.artima.com/cppsource/safebool.html https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:29: #include "bindings/core/v8/ScriptCallStack.h" There should be a way to eliminate this. I still don't see why it's necessary, but if it really is necessary, then probably something needs to be pushed down into ExecutionContext. FrameView does not have any javascript bindings, so it's a layering violation to include binding code here. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2421: if (lifecycle().state() < DocumentLifecycle::LayoutClean) This makes me uneasy. If you're going to call updateStyleAndLayoutIfNeededRecursive() before entering this method, then this should be: DCHECK(lifecycle().state() >= DocumentLifecycle::LayoutClean); Basically, we want to enforce a strict invariant that the document is in a specific state when this method is called. We don't want to be flexible about that. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2426: while (resizeController.gatherObservations()) { This still doesn't seem right. What happens to the notifications that are generated but never delivered because the loop limit is exceeded? There should not be undelivered notifications when this method exits. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObservation.cpp (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObservation.cpp:31: // else treat no layout as an empty rect. Please make sure this behavior is discussed in the spec. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObservation.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObservation.h:18: class ResizeObservation : public GarbageCollectedFinalized<ResizeObservation> { No Finalized, no destructor; see preceding comment. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObservation.h:19: Still too many blank lines. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObservation.h:39: Member<ResizeObserver> m_observer; // Used for GC only. // This member is unused; its purpose is to maintain a strong reference to the observer, to prevent it from being garbage-collected. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserver.cpp (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:28: if (observerMap && observerMap->find(this) != observerMap->end()) if (observerMap && observerMap->contains(this)) https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:40: m_observations.remove(m_observations.find((*observation).value)); m_observations.remove(observation->value) https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:47: while (!m_observations.isEmpty()) { Vector<ResizeObservation> observations; copyToVector(m_observations, observations); m_observations->clear(); for (auto observation : observations) { Element* target = observation->target(); if (target) // Need this check because ResizeObservation::m_target is weak target->ensureResizeObserverData().remove(observation); } https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:59: m_activeObservations.clear(); See previous comment; this should ASSERT(m_activeObservations->isEmpty()) https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserver.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.h:9: #include "core/dom/Element.h" Move this to .cpp file. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.h:10: #include "core/observer/ResizeObserverEntry.h" ditto https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.h:23: class ResizeObserver final : public GarbageCollectedFinalized<ResizeObserver>, public ScriptWrappable { No Finalized, no destructors; see previous comment. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverController.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverController.h:16: class ResizeObserverController : public GarbageCollectedFinalized<ResizeObserverController> { No Finalized https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp:24: m_contentRect.setSize(layout->contentSize()); m_contentRect = layout->contentBoxRect(); https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:19: class ResizeObserverEntry final : public GarbageCollectedFinalized<ResizeObserverEntry>, public ScriptWrappable { No Finalized
Thanks for the well-thought out, and helpful comments. Thanks to you, now I know the difference between `auto` and `auto&`. The fixes are cooking. While waiting for the review, I've almost finished my test suite. Should I add it to this patch, or submit it separately?
This patch set contains: - fixes for CR issues - a LayoutTests test suite I've also fixed issues I've found by running the test suite: - Event loop has to be prodded (scheduleAnimation)in a couple of places, otherwise observations do not get delivered. - ResizeObserverController::deliverObservations was looping over m_observers, that can be modified in the loop. HeapHashSet does not allow this. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:693: bool resizeObserverController() const { return m_resizeObserverController; } On 2016/06/02 20:52:41, szager1 wrote: > I would still prefer that you honor existing convention, like these: > > StyleResolver* styleResolver() const; > StyleResolver& ensureStyleResolver() const; > > IntersectionObserverController* intersectionObserverController(); > IntersectionObserverController& ensureIntersectionObserverController(); > > Document& ensureTemplateDocument(); > Document* templateDocumentHost() { return m_templateDocumentHost; } > > EventTargetData* eventTargetData() override; > EventTargetData& ensureEventTargetData() override; > > NodeRareData* rareData() const; > NodeRareData& ensureRareData(); > > Some background on why we discourage implicit bool conversions from concrete > types: > > http://www.artima.com/cppsource/safebool.html Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:29: #include "bindings/core/v8/ScriptCallStack.h" On 2016/06/02 20:52:41, szager1 wrote: > There should be a way to eliminate this. I still don't see why it's necessary, > but if it really is necessary, then probably something needs to be pushed down > into ExecutionContext. > > FrameView does not have any javascript bindings, so it's a layering violation to > include binding code here. Done. I've just pulled master, and the new API no longer requires the stack. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2421: if (lifecycle().state() < DocumentLifecycle::LayoutClean) On 2016/06/02 20:52:41, szager1 wrote: > This makes me uneasy. If you're going to call > updateStyleAndLayoutIfNeededRecursive() before entering this method, then this > should be: > > DCHECK(lifecycle().state() >= DocumentLifecycle::LayoutClean); > > Basically, we want to enforce a strict invariant that the document is in a > specific state when this method is called. We don't want to be flexible about > that. Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2426: while (resizeController.gatherObservations()) { On 2016/06/02 20:52:41, szager1 wrote: > This still doesn't seem right. What happens to the notifications that are > generated but never delivered because the loop limit is exceeded? There should > not be undelivered notifications when this method exits. Done. I've added a clearObservations(). I was clearing observations in gatherObservations(). This is better, the strong pointers to Element are not being kept an extra cycle. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObservation.cpp (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObservation.cpp:31: // else treat no layout as an empty rect. On 2016/06/02 20:52:41, szager1 wrote: > Please make sure this behavior is discussed in the spec. Will do. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObservation.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObservation.h:18: class ResizeObservation : public GarbageCollectedFinalized<ResizeObservation> { On 2016/06/02 20:52:41, szager1 wrote: > No Finalized, no destructor; see preceding comment. Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObservation.h:19: On 2016/06/02 20:52:41, szager1 wrote: > Still too many blank lines. Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObservation.h:39: Member<ResizeObserver> m_observer; // Used for GC only. On 2016/06/02 20:52:41, szager1 wrote: > // This member is unused; its purpose is to maintain a strong reference to the > observer, to prevent it from being garbage-collected. Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserver.cpp (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:28: if (observerMap && observerMap->find(this) != observerMap->end()) On 2016/06/02 20:52:41, szager1 wrote: > if (observerMap && observerMap->contains(this)) Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:40: m_observations.remove(m_observations.find((*observation).value)); On 2016/06/02 20:52:41, szager1 wrote: > m_observations.remove(observation->value) Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:47: while (!m_observations.isEmpty()) { On 2016/06/02 20:52:41, szager1 wrote: > Vector<ResizeObservation> observations; > copyToVector(m_observations, observations); > m_observations->clear(); > for (auto observation : observations) { > Element* target = observation->target(); > if (target) // Need this check because ResizeObservation::m_target is weak > target->ensureResizeObserverData().remove(observation); > } Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:59: m_activeObservations.clear(); On 2016/06/02 20:52:41, szager1 wrote: > See previous comment; this should ASSERT(m_activeObservations->isEmpty()) Done. Accomplished by calling clearObservations() from FrameView on error. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserver.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.h:9: #include "core/dom/Element.h" On 2016/06/02 20:52:41, szager1 wrote: > Move this to .cpp file. Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.h:10: #include "core/observer/ResizeObserverEntry.h" On 2016/06/02 20:52:41, szager1 wrote: > ditto Done. What was I thinking here? https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.h:23: class ResizeObserver final : public GarbageCollectedFinalized<ResizeObserver>, public ScriptWrappable { On 2016/06/02 20:52:41, szager1 wrote: > No Finalized, no destructors; see previous comment. This one has to be finalized because HeapLinkedHashSet m_observation requires finalization https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverController.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverController.h:16: class ResizeObserverController : public GarbageCollectedFinalized<ResizeObserverController> { On 2016/06/02 20:52:41, szager1 wrote: > No Finalized Done. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp:24: m_contentRect.setSize(layout->contentSize()); On 2016/06/02 20:52:41, szager1 wrote: > m_contentRect = layout->contentBoxRect(); Why? m_contentRect gets the size from layout, and position from padding. I feel that setting the size explicitly better communicates the intent. https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.h (right): https://codereview.chromium.org/2005593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:19: class ResizeObserverEntry final : public GarbageCollectedFinalized<ResizeObserverEntry>, public ScriptWrappable { On 2016/06/02 20:52:41, szager1 wrote: > No Finalized Done.
https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2461: while (resizeController.gatherObservations()) { This is a much clearer organization of the loop, thanks! https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2467: // Ensure notifications will get delivered in next cycle. This comment is obsolete, I think. Maybe just something about scheduling a new frame because at this point, the layout may be left in an inconsistent state. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2505: notifyResizeObserver(); Are you sure we don't want to run this when (phases == OnlyUpToLayoutClean)? This begs the question: in a world with ResizeObserver, what do we mean by "layout clean"? https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserver.cpp (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:36: LocalFrame* frame = target->document().frame(); nit: if (FrameView* frameView = document().view()) frameView->scheduleAnimation(); https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:58: for (auto it = m_observations.begin(); it != m_observations.end(); ++it) nit: m_observations.swap(observations); https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:61: m_observations.clear(); If you use swap(), this line is unnecessary. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:65: auto& observerMap = target->ensureResizeObserverData(); Can't you just: target->ensureResizeObserverData().remove(this); ? https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverController.cpp (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:25: hasObservations = hasObservations || observer->gatherObservations(); This will skip calling gatherObservations() if hasObservations is already true. Either reverse the operands, or (better): hasObservations |= observer->gatherObservations(); ... and you should probably add a test for this. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:32: ObserverSet observers; The idiomatic way to do this is: HeapVector<Member<ResizeObserver>> observers; copyToVector(m_observers, observers); https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.h (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:34: LayoutRect m_contentRect; This should be stored as a ClientRect*, because we need to preserve its identity in javascript, i.e.: entry.contentRect() === entry.contentRect();
All CR comments have been addressed. I've also rebased to current master. Good review, caught a few subtle gotchas. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2461: while (resizeController.gatherObservations()) { On 2016/07/11 18:33:47, szager1 wrote: > This is a much clearer organization of the loop, thanks! Done. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2467: // Ensure notifications will get delivered in next cycle. On 2016/07/11 18:33:47, szager1 wrote: > This comment is obsolete, I think. Maybe just something about scheduling a new > frame because at this point, the layout may be left in an inconsistent state. The comment is meaningful. Without scheduleAnimation(), notifications will not get delivered until next redraw. I can remove the comment if you'd like. I put the comment in because this was a bug fix I put in after a test failed. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2505: notifyResizeObserver(); On 2016/07/11 18:33:47, szager1 wrote: > Are you sure we don't want to run this when (phases == OnlyUpToLayoutClean)? > This begs the question: in a world with ResizeObserver, what do we mean by > "layout clean"? I believe that ResizeObserver should not run if phases == OnlyUpToLayoutClean According to spec, ResizeObserver only notifies before paint. OnlyUpToLayoutClean does not paint. I've looked at callers that trigger OnlyUpToLayoutClean condition, and only one is FrameSelection::unclippedBounds() https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserver.cpp (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:36: LocalFrame* frame = target->document().frame(); On 2016/07/11 18:33:47, szager1 wrote: > nit: > > if (FrameView* frameView = document().view()) > frameView->scheduleAnimation(); Done. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:58: for (auto it = m_observations.begin(); it != m_observations.end(); ++it) On 2016/07/11 18:33:47, szager1 wrote: > nit: > > m_observations.swap(observations); Done. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:61: m_observations.clear(); On 2016/07/11 18:33:47, szager1 wrote: > If you use swap(), this line is unnecessary. Done. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserver.cpp:65: auto& observerMap = target->ensureResizeObserverData(); Wow, you can. Still rusty with my C++ collections. On 2016/07/11 18:33:47, szager1 wrote: > Can't you just: > > target->ensureResizeObserverData().remove(this); > > ? https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverController.cpp (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:25: hasObservations = hasObservations || observer->gatherObservations(); On 2016/07/11 18:33:47, szager1 wrote: > This will skip calling gatherObservations() if hasObservations is already true. > Either reverse the operands, or (better): > > hasObservations |= observer->gatherObservations(); > > ... and you should probably add a test for this. Good catch. I've reversed the operands, since |= could be optimizing my gatherObservations away. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:32: ObserverSet observers; On 2016/07/11 18:33:47, szager1 wrote: > The idiomatic way to do this is: > > HeapVector<Member<ResizeObserver>> observers; > copyToVector(m_observers, observers); Done. A question: m_observers is a collection of WeakMembers observers is a collection of Members This code will prevent Observers from being GCd until the end of this routine. I assume this does not matter? https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.h (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverEntry.h:34: LayoutRect m_contentRect; On 2016/07/11 18:33:47, szager1 wrote: > This should be stored as a ClientRect*, because we need to preserve its identity > in javascript, i.e.: > > entry.contentRect() === entry.contentRect(); Did not think of that. Thanks.
lgtm, great work! https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2467: // Ensure notifications will get delivered in next cycle. On 2016/07/11 23:18:58, atotic1 wrote: > On 2016/07/11 18:33:47, szager1 wrote: > > This comment is obsolete, I think. Maybe just something about scheduling a > new > > frame because at this point, the layout may be left in an inconsistent state. > > The comment is meaningful. Without scheduleAnimation(), notifications will not > get delivered until next redraw. I can remove the comment if you'd like. > I put the comment in because this was a bug fix I put in after a test failed. But just a few lines up, you call resizeController.clearObservations(). So presumably, at this point, there won't be any notifications left for next frame (which is, I think, the right thing to do). https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2505: notifyResizeObserver(); On 2016/07/11 23:18:57, atotic1 wrote: > On 2016/07/11 18:33:47, szager1 wrote: > > Are you sure we don't want to run this when (phases == OnlyUpToLayoutClean)? > > This begs the question: in a world with ResizeObserver, what do we mean by > > "layout clean"? > > I believe that ResizeObserver should not run if phases == OnlyUpToLayoutClean > According to spec, ResizeObserver only notifies before paint. > OnlyUpToLayoutClean does not paint. > > I've looked at callers that trigger OnlyUpToLayoutClean condition, > and only one is FrameSelection::unclippedBounds() Good enough for me, I just wanted to make sure you were thinking about it. https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/observer/ResizeObserverController.cpp (right): https://codereview.chromium.org/2005593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/observer/ResizeObserverController.cpp:32: ObserverSet observers; On 2016/07/11 23:18:58, atotic1 wrote: > On 2016/07/11 18:33:47, szager1 wrote: > > The idiomatic way to do this is: > > > > HeapVector<Member<ResizeObserver>> observers; > > copyToVector(m_observers, observers); > > Done. A question: > m_observers is a collection of WeakMembers > observers is a collection of Members > > This code will prevent Observers from being GCd until the end of this routine. > I assume this does not matter? Correct: a GC can never happen during the execution of this method; and even if it did, you would still want to deliver pending notifications, even if that meant keeping objects alive a bit longer.
The CQ bit was checked by atotic@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by atotic@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== First stab at ResizeObserver. It mostly works, with few outstanding issues: - Notification loop breaks on numeric limit. Should use depth limit. - Notification loop polls for size changes. Ojan would like push. - ResizeObserverEntry should have DOMReadOnlyRect, not clientRect. DOMReadOnlyRect is still behind a flag FrameView::notifyResizeObserver loop needs review by someone who understands eventloop BUG=612962 ========== to ========== This is initial ResizeObserver implementation of the spec https://github.com/WICG/ResizeObserver/, with test cases. This CR does not completely implement the spec. The differences will be resolved in upcoming CRs. The differences are: 1) Notification loop breaks on numeric limit. Should use depth limit. 2) Notification loop polls for size changes. Push is more performant. 3) ResizeObserverEntry should have DOMRectReadOnly, not clientRect. DOMRectReadOnly is still behind a flag, will fix when it lands. BUG=612962 ==========
atotic@google.com changed reviewers: + eae@chromium.org, esprehn@chromium.org
Hi eae and esprehn, Ian suggested that you'd be good owner reviewers for ResizeObserver. I got a lgtm from szager, and would love to land this. If another OWNER can review better, please add. Aleks
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
This looks great! I have a few questions however. https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/resize-observer/notify.html (right): https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/resize-observer/notify.html:130: t1.style.paddingTop = "10px"; It would be nice to have a test with borders and margins, even if that doesn't change things. https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2512: if (!m_frame->document()->resizeObserverController()) When would notifyResizeObserver ever be called if the document doesn't have a resizeObserverController? A comment explaining the edge case would be helpful! https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2524: // Report the error. Please remove redundant comment. https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/observer/ResizeObserverController.h (right): https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/observer/ResizeObserverController.h:20: static const size_t kRenderLoopLimit = 16; A comment explaining the limit would be helpful. https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp (right): https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp:26: layout->paddingLeft(), layout->paddingTop(), Shouldn't the location include borderLeft and borderTop? contentWidth explicitly subtracts the borders (as well as the paddings). If so you can use ClientRect::create(FloatRect(layout->contentBoxRect()));
Hmm I'd like to land the code that pushes the resizes down the tree instead of the loop limit of 16. We don't want that in the shipping product and the structure of the code will be different to keep pushing stuff down. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hmm I'd like to land the code that pushes the resizes down the tree instead of the loop limit of 16. We don't want that in the shipping product and the structure of the code will be different to keep pushing stuff down. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2016/07/14 at 02:32:33, esprehn wrote: > Hmm I'd like to land the code that pushes the resizes down the tree instead > of the loop limit of 16. We don't want that in the shipping product and the > structure of the code will be different to keep pushing stuff down. > That is its not just depth limit, I think we want to actually start one level deeper in the tree each time? Also this patch is so massive, can we land it in smaller bits like just the interface and idl that has no behavior, then add the controller, then the methods, etc? Then we can also write unit tests as we go. Like your ResizeObserverEntry can be easily unit tested today. :)
On 2016/07/14 02:38:26, esprehn wrote: > Also this patch is so massive, can we land it in smaller bits like just the > interface and idl that has no behavior, then add the controller, then the > methods, etc? Then we can also write unit tests as we go. Like your > ResizeObserverEntry can be easily unit tested today. :) Had f2f with esprehn. This patch will be broken into smaller patches: 1) IDLs with stubs 2) ElementData hookup, ResizeObserverController stubs 3) Fill in IDL stubs, ResizeObserverEntry C++ unit tests 4) ResizeObserverController gathering of observations. 5) ResizeObserverController delivering observations 6) Hook up ResizeObserverController to FrameView polling style + e2e tests 7) Replace FrameView polling with push in_reply_to: 5728757302165504 send_mail: 1 subject: Initial ResizeObserver implementation
Replies to eae comments. https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/resize-observer/notify.html (right): https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/resize-observer/notify.html:130: t1.style.paddingTop = "10px"; On 2016/07/13 23:39:47, eae wrote: > It would be nice to have a test with borders and margins, even if that doesn't > change things. Done. https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2512: if (!m_frame->document()->resizeObserverController()) On 2016/07/13 23:39:47, eae wrote: > When would notifyResizeObserver ever be called if the document doesn't have a > resizeObserverController? A comment explaining the edge case would be helpful! Controller is allocated only if ResizeObserver is created. This is a common case. https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2524: // Report the error. On 2016/07/13 23:39:47, eae wrote: > Please remove redundant comment. Done. https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/observer/ResizeObserverController.h (right): https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/observer/ResizeObserverController.h:20: static const size_t kRenderLoopLimit = 16; On 2016/07/13 23:39:47, eae wrote: > A comment explaining the limit would be helpful. This limit will go away in the final checkin. https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp (right): https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp:26: layout->paddingLeft(), layout->paddingTop(), On 2016/07/13 23:39:47, eae wrote: > Shouldn't the location include borderLeft and borderTop? contentWidth explicitly > subtracts the borders (as well as the paddings). > > If so you can use ClientRect::create(FloatRect(layout->contentBoxRect())); Location does not include borderLeft/Top. See discussion at https://github.com/WICG/ResizeObserver/issues/6
On 2016/07/15 01:43:17, atotic1 wrote: > Replies to eae comments. > > https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/resize-observer/notify.html (right): > > https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/resize-observer/notify.html:130: > t1.style.paddingTop = "10px"; > On 2016/07/13 23:39:47, eae wrote: > > It would be nice to have a test with borders and margins, even if that doesn't > > change things. > > Done. > > https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/FrameView.cpp:2512: if > (!m_frame->document()->resizeObserverController()) > On 2016/07/13 23:39:47, eae wrote: > > When would notifyResizeObserver ever be called if the document doesn't have a > > resizeObserverController? A comment explaining the edge case would be helpful! > > Controller is allocated only if ResizeObserver is created. This is a common > case. Thanks for clarifying! > https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/FrameView.cpp:2524: // Report the error. > On 2016/07/13 23:39:47, eae wrote: > > Please remove redundant comment. > > Done. > > https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/observer/ResizeObserverController.h (right): > > https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/observer/ResizeObserverController.h:20: static > const size_t kRenderLoopLimit = 16; > On 2016/07/13 23:39:47, eae wrote: > > A comment explaining the limit would be helpful. > > This limit will go away in the final checkin. Great! > > https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp (right): > > https://codereview.chromium.org/2005593002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp:26: > layout->paddingLeft(), layout->paddingTop(), > On 2016/07/13 23:39:47, eae wrote: > > Shouldn't the location include borderLeft and borderTop? contentWidth > explicitly > > subtracts the borders (as well as the paddings). > > > > If so you can use ClientRect::create(FloatRect(layout->contentBoxRect())); > > Location does not include borderLeft/Top. See discussion at > https://github.com/WICG/ResizeObserver/issues/6 Ah, thanks for pointing that out. LGTM |
