Some questions about the implicit root logic, and some tiny nits, but other than that I think this is ready for a real review. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.cpp File Source/WebCore/dom/Document.cpp (right): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7375: if (!root || (root->isConnected() && &root->document() == this)) For the null root case, should we also check if |this| is the top-level document? https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7424: // FIXME: test with zoom. Is testing with zoom worth doing now (perhaps leaving the work of fixing behavior with zoom to later)? https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7435: // FIXME: handle writing modes. Is this what you were mentioning earlier, that writing modes are ignored? https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7517: } Here too, I wonder if we need to check if |this| is the top-level root when observer.root() is null. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7592: if (frameView->frame().isMainFrame()) This is another place where I'm confused about implicit roots (or maybe misunderstanding the spec). I'd have thought that if we're using an implicit root, then this document must be the top-level document. So the condition here would be an assertion rather than an if statement. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.h File Source/WebCore/dom/Document.h (left): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.h... Source/WebCore/dom/Document.h:714: Lots of unrelated whitespace changes (here and in other files) which should probably be removed before a real WebKit review. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.h File Source/WebCore/dom/Document.h (right): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.h... Source/WebCore/dom/Document.h:237: #if ENABLE(INTERSECTION_OBSERVER) WebKit has recently been moving towards runtime-enabled features over compile-time, since that lets you run tests on the bots and/or have a flag for enabling it in Safari Tech Preview. But we can leave this for now and see what smfr thinks. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Element.cpp File Source/WebCore/dom/Element.cpp (right): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Element.cp... Source/WebCore/dom/Element.cpp:3232: // If target is in thiss internal [[ObservationTargets]] slot, return. Typo ("thiss"), also a couple spots below. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Element.cp... Source/WebCore/dom/Element.cpp:3247: // FIXME. Is this something like scheduling layout on the root document?
https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.cpp File Source/WebCore/dom/Document.cpp (right): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7375: if (!root || (root->isConnected() && &root->document() == this)) On 2018/06/25 17:54:37, ajuma wrote: > For the null root case, should we also check if |this| is the top-level > document? Please see comment later about computeAbsoluteTargetIntersection. I think this could work. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7424: // FIXME: test with zoom. On 2018/06/25 17:54:37, ajuma wrote: > Is testing with zoom worth doing now (perhaps leaving the work of fixing > behavior with zoom to later)? I checked zoom, and it seemed wrong. I attempted to fix this, but I'm not confident I'm on the right track and would like to chat with you at some point. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7435: // FIXME: handle writing modes. On 2018/06/25 17:54:37, ajuma wrote: > Is this what you were mentioning earlier, that writing modes are ignored? Yeah, I'm not confident that this code is correct. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7517: } On 2018/06/25 17:54:37, ajuma wrote: > Here too, I wonder if we need to check if |this| is the top-level root when > observer.root() is null. This appears to work without the check because of the code in computeAbsoluteTargetIntersection which, in the !isMainFrame case, converts to window coordinates. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7592: if (frameView->frame().isMainFrame()) On 2018/06/25 17:54:38, ajuma wrote: > This is another place where I'm confused about implicit roots (or maybe > misunderstanding the spec). I'd have thought that if we're using an implicit > root, then this document must be the top-level document. So the condition here > would be an assertion rather than an if statement. I think this does what was intended (see comment above) but I think we may want a different conditional https://w3c.github.io/IntersectionObserver/#dom-intersectionobserverentry-roo.... See also https://github.com/w3c/IntersectionObserver/issues/170#issuecom%20ment-257174868 https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.h File Source/WebCore/dom/Document.h (left): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.h... Source/WebCore/dom/Document.h:714: On 2018/06/25 17:54:38, ajuma wrote: > Lots of unrelated whitespace changes (here and in other files) which should > probably be removed before a real WebKit review. Removed. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.h File Source/WebCore/dom/Document.h (right): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.h... Source/WebCore/dom/Document.h:237: #if ENABLE(INTERSECTION_OBSERVER) On 2018/06/25 17:54:38, ajuma wrote: > WebKit has recently been moving towards runtime-enabled features over > compile-time, since that lets you run tests on the bots and/or have a flag for > enabling it in Safari Tech Preview. But we can leave this for now and see what > smfr thinks. sg. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Element.cpp File Source/WebCore/dom/Element.cpp (right): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Element.cp... Source/WebCore/dom/Element.cpp:3232: // If target is in thiss internal [[ObservationTargets]] slot, return. On 2018/06/25 17:54:38, ajuma wrote: > Typo ("thiss"), also a couple spots below. Changed to this's. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Element.cp... Source/WebCore/dom/Element.cpp:3247: // FIXME. On 2018/06/25 17:54:38, ajuma wrote: > Is this something like scheduling layout on the root document? I thought it was scheduling something that would guarantee that we'd call updateIntersectionObservations. I could add a corresponding scheduleFOO function to Document and call it here, maybe?
https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.cpp File Source/WebCore/dom/Document.cpp (right): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Document.c... Source/WebCore/dom/Document.cpp:7424: // FIXME: test with zoom. On 2018/07/03 15:22:28, vollick wrote: > On 2018/06/25 17:54:37, ajuma wrote: > > Is testing with zoom worth doing now (perhaps leaving the work of fixing > > behavior with zoom to later)? > > I checked zoom, and it seemed wrong. I attempted to fix this, but I'm not > confident I'm on the right track and would like to chat with you at some point. Sure, sgtm. https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Element.cpp File Source/WebCore/dom/Element.cpp (right): https://codereview.appspot.com/349730043/diff/1/Source/WebCore/dom/Element.cp... Source/WebCore/dom/Element.cpp:3247: // FIXME. On 2018/07/03 15:22:29, vollick wrote: > On 2018/06/25 17:54:38, ajuma wrote: > > Is this something like scheduling layout on the root document? > > I thought it was scheduling something that would guarantee that we'd call > updateIntersectionObservations. I could add a corresponding scheduleFOO function > to Document and call it here, maybe? That sounds like a reasonable approach. https://codereview.appspot.com/349730043/diff/80001/Source/WebCore/dom/Docume... File Source/WebCore/dom/Document.cpp (right): https://codereview.appspot.com/349730043/diff/80001/Source/WebCore/dom/Docume... Source/WebCore/dom/Document.cpp:7438: // Page zoom should not affect visiual viewport, so we will only set the Typo: visual https://codereview.appspot.com/349730043/diff/80001/Source/WebCore/dom/Docume... Source/WebCore/dom/Document.cpp:7439: // scale factor here, for the case where the root element is specified. See the "Coordinate systems" comment in FrameView.h (it's around line 426). Maybe you could rephrase this comment and the math done in this function using the terminology defined there?
