|
|
Created:
4 years, 9 months ago by szager1 Modified:
4 years, 9 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src@intersection-observer-inclusive-geometry Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntersectionObserver: use edge-inclusive geometry for intersections.
Split this out from:
https://codereview.chromium.org/1817693002/
BUG=540528
R=ojan@chromium.org,chrishtr@chromium.org
Committed: https://crrev.com/1275d3aca4b5dab9bdaeac8255fd54fe58649182
Cr-Commit-Position: refs/heads/master@{#383371}
Patch Set 1 #
Total comments: 9
Patch Set 2 : refactor threshold index calculating code, fix test #
Total comments: 10
Patch Set 3 : tiny tweak #Patch Set 4 : tweaks #
Total comments: 3
Patch Set 5 : Get rid of enum #Patch Set 6 : refactor #Depends on Patchset: Messages
Total messages: 22 (2 generated)
https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/edge.html (right): https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/edge.html:1: <!DOCTYPE html> edge.html is not such a great name. edge-inclusive-intersection.html ? https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/same-document-zero-size-target.html (right): https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/same-document-zero-size-target.html:9: description("Iintersection observer test with zero-size target element."); typo. Also, what is the change to this file for? https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:181: if (m_shouldReportRootBounds) Why this new conditional? https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:202: unsigned newThresholdIndex = observer().firstThresholdGreaterThan(newVisibleRatio); Alternatively, explain to firstThresholdGreaterThan how to understand the doesIntersect parameter?
https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/edge.html (right): https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/edge.html:1: <!DOCTYPE html> On 2016/03/24 21:49:12, chrishtr wrote: > edge.html is not such a great name. edge-inclusive-intersection.html ? Done. https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/intersection-observer/same-document-zero-size-target.html (right): https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/intersection-observer/same-document-zero-size-target.html:9: description("Iintersection observer test with zero-size target element."); On 2016/03/24 21:49:12, chrishtr wrote: > typo. > > Also, what is the change to this file for? Whoops, I did some reformatting of the test that is meant for another patch. I will remove the changes to this file. Note that the changes to the expectations are correct. Previously, IntersectionObserver would expand a zero-size target rect by one pixel prior to calculating the intersection, so the result was also expanded by one pixel. With this patch, the one-pixel expansion doesn't happen. https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:181: if (m_shouldReportRootBounds) On 2016/03/24 21:49:12, chrishtr wrote: > Why this new conditional? This is just a slight refactoring to avoid doing unnecessary work; previously, the rootRect was zero-ed after this method returned. https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:202: unsigned newThresholdIndex = observer().firstThresholdGreaterThan(newVisibleRatio); On 2016/03/24 21:49:12, chrishtr wrote: > Alternatively, explain to firstThresholdGreaterThan how to understand the > doesIntersect parameter? This got really subtle; I rewrote this section with some comments to explain the magic.
https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/1826323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:181: if (m_shouldReportRootBounds) On 2016/03/24 at 23:14:08, szager1 wrote: > On 2016/03/24 21:49:12, chrishtr wrote: > > Why this new conditional? > > This is just a slight refactoring to avoid doing unnecessary work; previously, the rootRect was zero-ed after this method returned. I'm probably being stupid, can you show me where?
https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (left): https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:199: IntRect* rootBoundsPointer = m_shouldReportRootBounds ? &snappedRootBounds : nullptr; Here.
https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/intersection-observer/same-document-zero-size-target.html (right): https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/intersection-observer/same-document-zero-size-target.html:31: shouldBeEqualToNumber("entries[0].boundingClientRect.bottom", 408); Why did these change? https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:196: // - If target rect is zero area, only two states are recognized: Good to mention that zero-area means either width or height is 0, not that it's zero-sized. https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:198: // - 1 means intersecting. Change to an enum.
https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (left): https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:199: IntRect* rootBoundsPointer = m_shouldReportRootBounds ? &snappedRootBounds : nullptr; On 2016/03/24 at 23:33:09, szager1 wrote: > Here. ah https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:184: geometry.rootRect = LayoutRect(); Might as well kill this else clause.
https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/intersection-observer/same-document-zero-size-target.html (right): https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/intersection-observer/same-document-zero-size-target.html:31: shouldBeEqualToNumber("entries[0].boundingClientRect.bottom", 408); On 2016/03/24 23:33:11, chrishtr wrote: > Why did these change? Previously, IntersectionObserver would expand a zero-size target rect by one pixel prior to calculating the intersection, so the result was also expanded by one pixel. With this patch, the one-pixel expansion doesn't happen. https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:184: geometry.rootRect = LayoutRect(); On 2016/03/24 23:35:29, chrishtr wrote: > Might as well kill this else clause. Done. https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:196: // - If target rect is zero area, only two states are recognized: On 2016/03/24 23:33:11, chrishtr wrote: > Good to mention that zero-area means either width or height is 0, not that it's > zero-sized. Done. https://codereview.chromium.org/1826323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:198: // - 1 means intersecting. On 2016/03/24 23:33:11, chrishtr wrote: > Change to an enum. OK, although it seems weird to use an enum when, in the non-zero-area-target case, it's not an enum (it's an index into a list of thresholds).
https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:220: newThresholdIndex = observer().firstThresholdGreaterThan(newVisibleRatio); Oh I see. It is weird to have an enum combined with an index. You can revert the enum change if you like. Aren't there two interpretations of 0 and 1 in this implementation? AIUI 0 can mean not intersecting, or less than all of the thresholds. 1 means intersecting but zero-area, or larger than the 0th threshold but less than the 1st. Isn't that a problem? https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right): https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.h:32: unsigned lastThresholdIndex() const { return m_lastThresholdIndex; } This is unused, delete?
https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:220: newThresholdIndex = observer().firstThresholdGreaterThan(newVisibleRatio); On 2016/03/25 14:54:47, chrishtr wrote: > Oh I see. It is weird to have an enum combined with an index. You can revert the > enum change if you like. > > > Aren't there two interpretations of 0 and 1 in this implementation? AIUI 0 can > mean not intersecting, or less than all of the thresholds. 1 means intersecting > but zero-area, or larger than the 0th threshold but less than the 1st. Isn't > that a problem? I think it's important to keep in mind that the purpose of this code is correctly identify threshold crossings; newThresholdIndex is a mostly-but-not-completely accurate variable name. There are a couple of threshold crossings that require special handling: - The zero threshold has special meaning in the spec: it is crossed whenever target and root transition from not intersecting to intersecting in any way (including coincident edge intersections). - If the target rect has zero area, then only two states are recognized: intersecting (threshold index 0) and not intersecting (threshold index 1). So yes, you are correct that a threshold index of 0 can mean two different things: not intersecting at all, or intersecting but newVisibleRatio is smaller than the first threshold (which consequently must not be a zero threshold).
On 2016/03/25 17:03:11, szager1 wrote: > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): > > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:220: > newThresholdIndex = observer().firstThresholdGreaterThan(newVisibleRatio); > On 2016/03/25 14:54:47, chrishtr wrote: > > Oh I see. It is weird to have an enum combined with an index. You can revert > the > > enum change if you like. > > > > > > Aren't there two interpretations of 0 and 1 in this implementation? AIUI 0 can > > mean not intersecting, or less than all of the thresholds. 1 means > intersecting > > but zero-area, or larger than the 0th threshold but less than the 1st. Isn't > > that a problem? > > I think it's important to keep in mind that the purpose of this code is > correctly identify threshold crossings; newThresholdIndex is a > mostly-but-not-completely accurate variable name. Would it be clearer to rename the variables to intersectionState or something similar, to signify that it's not exactly the same as threshold index?
On 2016/03/25 at 17:04:40, szager wrote: > On 2016/03/25 17:03:11, szager1 wrote: > > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): > > > > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:220: > > newThresholdIndex = observer().firstThresholdGreaterThan(newVisibleRatio); > > On 2016/03/25 14:54:47, chrishtr wrote: > > > Oh I see. It is weird to have an enum combined with an index. You can revert > > the > > > enum change if you like. > > > > > > > > > Aren't there two interpretations of 0 and 1 in this implementation? AIUI 0 can > > > mean not intersecting, or less than all of the thresholds. 1 means > > intersecting > > > but zero-area, or larger than the 0th threshold but less than the 1st. Isn't > > > that a problem? > > > > I think it's important to keep in mind that the purpose of this code is > > correctly identify threshold crossings; newThresholdIndex is a > > mostly-but-not-completely accurate variable name. > > Would it be clearer to rename the variables to intersectionState or something similar, to signify that it's not exactly the same as threshold index? My main concern is that this index is used to determine whether state changed, and hence cause observation entries to be queued. What if a transition occurs from state 1 (intersecting but zero-area) to state 1 (larger than the 0th threshold but less than the 1st)? This will result in no observation being queued. Is that incorrect?
On 2016/03/25 18:11:16, chrishtr wrote: > On 2016/03/25 at 17:04:40, szager wrote: > > On 2016/03/25 17:03:11, szager1 wrote: > > > > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): > > > > > > > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:220: > > > newThresholdIndex = observer().firstThresholdGreaterThan(newVisibleRatio); > > > On 2016/03/25 14:54:47, chrishtr wrote: > > > > Oh I see. It is weird to have an enum combined with an index. You can > revert > > > the > > > > enum change if you like. > > > > > > > > > > > > Aren't there two interpretations of 0 and 1 in this implementation? AIUI 0 > can > > > > mean not intersecting, or less than all of the thresholds. 1 means > > > intersecting > > > > but zero-area, or larger than the 0th threshold but less than the 1st. > Isn't > > > > that a problem? > > > > > > I think it's important to keep in mind that the purpose of this code is > > > correctly identify threshold crossings; newThresholdIndex is a > > > mostly-but-not-completely accurate variable name. > > > > Would it be clearer to rename the variables to intersectionState or something > similar, to signify that it's not exactly the same as threshold index? > > My main concern is that this index is used to determine whether state changed, > and hence cause observation entries to be queued. What if a transition occurs > from state 1 (intersecting but zero-area) to state 1 (larger than the 0th > threshold but less than the 1st)? This will result in no observation being > queued. Is that incorrect? That is incorrect. Take a look at IntersectionObserver::firstThresholdGreaterThan if you need to convince yourself.
On 2016/03/25 at 18:20:06, szager wrote: > On 2016/03/25 18:11:16, chrishtr wrote: > > On 2016/03/25 at 17:04:40, szager wrote: > > > On 2016/03/25 17:03:11, szager1 wrote: > > > > > > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): > > > > > > > > > > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:220: > > > > newThresholdIndex = observer().firstThresholdGreaterThan(newVisibleRatio); > > > > On 2016/03/25 14:54:47, chrishtr wrote: > > > > > Oh I see. It is weird to have an enum combined with an index. You can > > revert > > > > the > > > > > enum change if you like. > > > > > > > > > > > > > > > Aren't there two interpretations of 0 and 1 in this implementation? AIUI 0 > > can > > > > > mean not intersecting, or less than all of the thresholds. 1 means > > > > intersecting > > > > > but zero-area, or larger than the 0th threshold but less than the 1st. > > Isn't > > > > > that a problem? > > > > > > > > I think it's important to keep in mind that the purpose of this code is > > > > correctly identify threshold crossings; newThresholdIndex is a > > > > mostly-but-not-completely accurate variable name. > > > > > > Would it be clearer to rename the variables to intersectionState or something > > similar, to signify that it's not exactly the same as threshold index? > > > > My main concern is that this index is used to determine whether state changed, > > and hence cause observation entries to be queued. What if a transition occurs > > from state 1 (intersecting but zero-area) to state 1 (larger than the 0th > > threshold but less than the 1st)? This will result in no observation being > > queued. Is that incorrect? > > That is incorrect. Take a look at IntersectionObserver::firstThresholdGreaterThan if you need to convince yourself. Ok. Per offline conversation, it would be clearer to rewrite as: if (geometry.targetRect.isEmpty()) { do 0/1 semantics for that case } else { do other stuff involving firstThresholdGreaterThan }
On 2016/03/25 18:29:19, chrishtr wrote: > On 2016/03/25 at 18:20:06, szager wrote: > > On 2016/03/25 18:11:16, chrishtr wrote: > > > On 2016/03/25 at 17:04:40, szager wrote: > > > > On 2016/03/25 17:03:11, szager1 wrote: > > > > > > > > > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1826323002/diff/60001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:220: > > > > > newThresholdIndex = > observer().firstThresholdGreaterThan(newVisibleRatio); > > > > > On 2016/03/25 14:54:47, chrishtr wrote: > > > > > > Oh I see. It is weird to have an enum combined with an index. You can > > > revert > > > > > the > > > > > > enum change if you like. > > > > > > > > > > > > > > > > > > Aren't there two interpretations of 0 and 1 in this implementation? > AIUI 0 > > > can > > > > > > mean not intersecting, or less than all of the thresholds. 1 means > > > > > intersecting > > > > > > but zero-area, or larger than the 0th threshold but less than the 1st. > > > Isn't > > > > > > that a problem? > > > > > > > > > > I think it's important to keep in mind that the purpose of this code is > > > > > correctly identify threshold crossings; newThresholdIndex is a > > > > > mostly-but-not-completely accurate variable name. > > > > > > > > Would it be clearer to rename the variables to intersectionState or > something > > > similar, to signify that it's not exactly the same as threshold index? > > > > > > My main concern is that this index is used to determine whether state > changed, > > > and hence cause observation entries to be queued. What if a transition > occurs > > > from state 1 (intersecting but zero-area) to state 1 (larger than the 0th > > > threshold but less than the 1st)? This will result in no observation being > > > queued. Is that incorrect? > > > > That is incorrect. Take a look at > IntersectionObserver::firstThresholdGreaterThan if you need to convince > yourself. > > Ok. Per offline conversation, it would be clearer to rewrite as: > > if (geometry.targetRect.isEmpty()) { > do 0/1 semantics for that case > } else { > do other stuff involving firstThresholdGreaterThan > }
On 2016/03/25 18:29:19, chrishtr wrote: > > Ok. Per offline conversation, it would be clearer to rewrite as: > > if (geometry.targetRect.isEmpty()) { > do 0/1 semantics for that case > } else { > do other stuff involving firstThresholdGreaterThan > } Done, PTAL.
The CQ bit was checked by chrishtr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826323002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826323002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== IntersectionObserver: use edge-inclusive geometry for intersections. Split this out from: https://codereview.chromium.org/1817693002/ BUG=540528 R=ojan@chromium.org,chrishtr@chromium.org ========== to ========== IntersectionObserver: use edge-inclusive geometry for intersections. Split this out from: https://codereview.chromium.org/1817693002/ BUG=540528 R=ojan@chromium.org,chrishtr@chromium.org Committed: https://crrev.com/1275d3aca4b5dab9bdaeac8255fd54fe58649182 Cr-Commit-Position: refs/heads/master@{#383371} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1275d3aca4b5dab9bdaeac8255fd54fe58649182 Cr-Commit-Position: refs/heads/master@{#383371} |