Code review - Issue 349730043: WIP - wiphttps://codereview.appspot.com/2018-07-03T18:21:48+00:00rietveld
Message from unknown
2018-06-25T15:06:15+00:00vollickurn:md5:28039669c444e6be059ee91f350a8fe5
Message from ajuma@chromium.org
2018-06-25T17:54:38+00:00ajumaurn:md5:6e3e7c14f454093302c6446ed7f4f160
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.cpp#newcode7375
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.cpp#newcode7424
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.cpp#newcode7435
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.cpp#newcode7517
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.cpp#newcode7592
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#oldcode714
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#newcode237
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.cpp#newcode3232
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.cpp#newcode3247
Source/WebCore/dom/Element.cpp:3247: // FIXME.
Is this something like scheduling layout on the root document?
Message from unknown
2018-07-03T14:47:49+00:00vollickurn:md5:90440f87aeb08de012bf47505b6e3378
Message from unknown
2018-07-03T15:15:34+00:00vollickurn:md5:e1ef03d67538d2d94bab0a1587006b1d
Message from unknown
2018-07-03T15:16:55+00:00vollickurn:md5:57ec23379fb7e10ef24e7b72b97a1711
Message from unknown
2018-07-03T15:21:11+00:00vollickurn:md5:f5f053e87c09dd01cc751249d1a3100f
Message from vollick@chromium.org
2018-07-03T15:22:29+00:00vollickurn:md5:3c02142456891dc9a869091d742549f3
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.cpp#newcode7375
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.cpp#newcode7424
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.cpp#newcode7435
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.cpp#newcode7517
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.cpp#newcode7592
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-rootbounds. 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#oldcode714
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#newcode237
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.cpp#newcode3232
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.cpp#newcode3247
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?
Message from ajuma@chromium.org
2018-07-03T18:21:48+00:00ajumaurn:md5:c0a0876226c4aad374dc6bb03d69ec7e
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.cpp#newcode7424
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.cpp#newcode3247
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/Document.cpp
File Source/WebCore/dom/Document.cpp (right):
https://codereview.appspot.com/349730043/diff/80001/Source/WebCore/dom/Document.cpp#newcode7438
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/Document.cpp#newcode7439
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?