Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(343)

Issue 1449623002: IntersectionObserver: second cut. (Closed)

Created:
5 years, 1 month ago by szager1
Modified:
4 years, 11 months ago
CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, Inactive, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, site-isolation-reviews_chromium.org, vivekg_samsung, vivekg
Base URL:
https://chromium.googlesource.com/chromium/src@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

IntersectionObserver: second cut. All major features are implemented, a few TODO's, and a few tests. Design doc: https://docs.google.com/document/d/1hLK0eyT5_BzyNS4OkjsnoqqFQDYCbKfyBinj94OnLiQ/edit BUG=540528 Committed: https://crrev.com/b5cff0041ec225f4eaac870e87d6189321754983 Cr-Commit-Position: refs/heads/master@{#368458}

Patch Set 1 #

Patch Set 2 : Clarify the tear-down path for observers and observations. #

Total comments: 39

Patch Set 3 : oilpan-ify #

Total comments: 6

Patch Set 4 : Use existing Document::m_weakFactory. #

Total comments: 2

Patch Set 5 : Oilpan-ed and kinda works #

Patch Set 6 : Refactor clipping code #

Total comments: 2

Patch Set 7 : Added tests, fixed intersection computation. #

Patch Set 8 : Fix rootMargin parsing #

Total comments: 8

Patch Set 9 : Implemented root margin #

Total comments: 241

Patch Set 10 : Comments addressed, every last one. #

Patch Set 11 : Added dispose() methods for expicit cleanup #

Total comments: 66

Patch Set 12 : Second round of esprehn comments addressed. #

Patch Set 13 : Add missing break, rebaseline, no config.h #

Total comments: 102

Patch Set 14 : Addressed haraken's comments #

Patch Set 15 : Addressed latest comments from esprehn #

Patch Set 16 : Remove bad ASSERT #

Total comments: 18

Patch Set 17 : sigbjornf nits #

Total comments: 37

Patch Set 18 : haraken nits #

Total comments: 2

Patch Set 19 : split out suspend/resume and root margin #

Patch Set 20 : Fix bad edit. #

Patch Set 21 : clean up teardown logic #

Total comments: 15

Patch Set 22 : more haraken nits #

Patch Set 23 : rebaseline #

Total comments: 2

Patch Set 24 : header nit, fix test expectation #

Patch Set 25 : test expectation #

Patch Set 26 : Add RuntimeEnabled flags to all idl's, fix test expectations. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+2061 lines, -39 lines) Patch
A third_party/WebKit/LayoutTests/intersection-observer/helper-functions.js View 1 2 3 4 5 6 7 8 9 1 chunk +37 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/iframe-no-root.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +87 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/iframe-no-root-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +40 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/iframe-root.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +107 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/iframe-root-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +42 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/multiple-thresholds.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +188 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/multiple-thresholds-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +109 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/same-document-no-root.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +80 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/same-document-no-root-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +41 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/same-document-root.html View 1 2 3 4 5 6 7 8 9 1 chunk +113 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/intersection-observer/same-document-root-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +54 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/resources/intersection-observer-subframe.html View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/resources/js-test.js View 1 2 3 4 5 6 7 8 9 1 chunk +17 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +13 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/core/v8/V8IntersectionObserverCallback.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +35 lines, -0 lines 0 comments Download
A + third_party/WebKit/Source/bindings/core/v8/V8IntersectionObserverCallback.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +10 lines, -13 lines 0 comments Download
A third_party/WebKit/Source/bindings/core/v8/custom/V8IntersectionObserverCustom.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +54 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/custom/custom.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/v8.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 5 chunks +15 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 4 chunks +19 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 4 chunks +22 lines, -2 lines 0 comments Download
A third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +49 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +103 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ElementRareData.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +15 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/ElementRareData.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +2 lines, -2 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObservation.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +54 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObservation.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +162 lines, -0 lines 2 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserver.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +62 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserver.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +237 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserver.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +19 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserverCallback.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +25 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserverController.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +42 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +66 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserverEntry.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +41 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserverEntry.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +33 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserverEntry.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +16 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/IntersectionObserverInit.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +12 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/FrameView.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/frame/FrameView.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 9 chunks +18 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 106 (16 generated)
szager1
Here it is: the basic architecture is there, but I literally haven't tried running an ...
5 years, 1 month ago (2015-11-14 00:15:35 UTC) #3
haraken
Here is a first round of comments :) - You don't need to use WillBe ...
5 years, 1 month ago (2015-11-16 00:41:35 UTC) #7
haraken
5 years, 1 month ago (2015-11-16 00:41:47 UTC) #9
eae
First pass, still trying to wrap my head around some of the logic. https://codereview.chromium.org/1449623002/diff/20001/third_party/WebKit/LayoutTests/fast/dom/intersection-observer.html File ...
5 years, 1 month ago (2015-11-16 23:34:45 UTC) #10
szager1
https://codereview.chromium.org/1449623002/diff/20001/third_party/WebKit/Source/core/dom/IntersectionObservation.h File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right): https://codereview.chromium.org/1449623002/diff/20001/third_party/WebKit/Source/core/dom/IntersectionObservation.h#newcode18 third_party/WebKit/Source/core/dom/IntersectionObservation.h:18: class IntersectionObservation : public RefCountedWillBeGarbageCollectedFinalized<IntersectionObservation> { On 2015/11/16 00:41:34, ...
5 years, 1 month ago (2015-11-17 01:50:04 UTC) #11
haraken
On 2015/11/17 01:50:04, szager1 wrote: > https://codereview.chromium.org/1449623002/diff/20001/third_party/WebKit/Source/core/dom/IntersectionObservation.h > File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right): > > https://codereview.chromium.org/1449623002/diff/20001/third_party/WebKit/Source/core/dom/IntersectionObservation.h#newcode18 > ...
5 years, 1 month ago (2015-11-17 01:56:43 UTC) #12
szager1
I'm working on making the new types garbage collected... https://codereview.chromium.org/1449623002/diff/20001/third_party/WebKit/LayoutTests/fast/dom/intersection-observer.html File third_party/WebKit/LayoutTests/fast/dom/intersection-observer.html (right): https://codereview.chromium.org/1449623002/diff/20001/third_party/WebKit/LayoutTests/fast/dom/intersection-observer.html#newcode61 third_party/WebKit/LayoutTests/fast/dom/intersection-observer.html:61: ...
5 years, 1 month ago (2015-11-19 19:15:38 UTC) #13
haraken
> https://codereview.chromium.org/1449623002/diff/20001/third_party/WebKit/Source/core/dom/IntersectionObserverEntry.h#newcode17 > third_party/WebKit/Source/core/dom/IntersectionObserverEntry.h:17: class > IntersectionObserverEntry : public > RefCountedWillBeGarbageCollectedFinalized<IntersectionObserverEntry>, public > ScriptWrappable { ...
5 years, 1 month ago (2015-11-19 23:39:14 UTC) #14
szager1
Oilpan-ified. PTAL
5 years, 1 month ago (2015-11-20 20:18:05 UTC) #15
dcheng
https://codereview.chromium.org/1449623002/diff/40001/third_party/WebKit/Source/core/dom/Document.h File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1449623002/diff/40001/third_party/WebKit/Source/core/dom/Document.h#newcode1401 third_party/WebKit/Source/core/dom/Document.h:1401: OwnPtrWillBeMember<WeakPtrFactory<Document>> m_weakPointerFactory; WeakPtrFactory is not a garbage collected object, ...
5 years, 1 month ago (2015-11-20 20:35:07 UTC) #17
szager1
https://codereview.chromium.org/1449623002/diff/40001/third_party/WebKit/Source/core/dom/Document.h File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1449623002/diff/40001/third_party/WebKit/Source/core/dom/Document.h#newcode1401 third_party/WebKit/Source/core/dom/Document.h:1401: OwnPtrWillBeMember<WeakPtrFactory<Document>> m_weakPointerFactory; On 2015/11/20 20:35:07, dcheng wrote: > WeakPtrFactory ...
5 years, 1 month ago (2015-11-20 22:02:23 UTC) #18
dcheng
https://codereview.chromium.org/1449623002/diff/40001/third_party/WebKit/Source/core/dom/Element.h File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/1449623002/diff/40001/third_party/WebKit/Source/core/dom/Element.h#newcode543 third_party/WebKit/Source/core/dom/Element.h:543: WeakPtr<Element> createWeakPtr(); On 2015/11/20 at 22:02:23, szager1 wrote: > ...
5 years, 1 month ago (2015-11-20 22:50:53 UTC) #19
szager1
On 2015/11/20 22:50:53, dcheng wrote: > > In general, I'm not thrilled about exposing weak ...
5 years, 1 month ago (2015-11-20 23:15:19 UTC) #20
szager1
PTAL I cleaned up the reference model, in the code and in the design doc: ...
5 years ago (2015-12-01 06:53:31 UTC) #21
szager1
Adding adamk and esprehn as reviewerss
5 years ago (2015-12-02 01:52:33 UTC) #23
szager1
Added tests. I'd like to get this landed, please take a look.
5 years ago (2015-12-08 02:14:24 UTC) #24
dcheng
I'm mostly reviewing this from a 'lifetime of things' perspective. I would really like to ...
5 years ago (2015-12-10 00:37:10 UTC) #25
szager1
https://codereview.chromium.org/1449623002/diff/140001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1449623002/diff/140001/third_party/WebKit/Source/core/dom/Document.cpp#newcode4965 third_party/WebKit/Source/core/dom/Document.cpp:4965: WeakPtrWillBeRawPtr<Document> Document::createWeakPtr() On 2015/12/10 00:37:09, dcheng wrote: > Having ...
5 years ago (2015-12-10 22:38:55 UTC) #26
szager1
On 2015/12/10 22:38:55, szager1 wrote: > https://codereview.chromium.org/1449623002/diff/140001/third_party/WebKit/Source/core/dom/Document.cpp > File third_party/WebKit/Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/1449623002/diff/140001/third_party/WebKit/Source/core/dom/Document.cpp#newcode4965 > ...
5 years ago (2015-12-10 22:40:29 UTC) #27
esprehn
https://codereview.chromium.org/1449623002/diff/100001/third_party/WebKit/Source/core/dom/Element.h File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/1449623002/diff/100001/third_party/WebKit/Source/core/dom/Element.h#newcode547 third_party/WebKit/Source/core/dom/Element.h:547: bool computeIntersection(Element*, LayoutRect&, LayoutRect&, LayoutRect&); const, also const Element* ...
5 years ago (2015-12-12 00:14:17 UTC) #28
ojan
I haven't had a chance to review the tests yet, but here's comments on the ...
5 years ago (2015-12-12 01:06:29 UTC) #29
esprehn
https://codereview.chromium.org/1449623002/diff/160001/third_party/WebKit/Source/core/dom/IntersectionObserver.h File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right): https://codereview.chromium.org/1449623002/diff/160001/third_party/WebKit/Source/core/dom/IntersectionObserver.h#newcode62 third_party/WebKit/Source/core/dom/IntersectionObserver.h:62: On 2015/12/12 at 01:06:29, ojan wrote: > On 2015/12/12 ...
5 years ago (2015-12-12 01:49:52 UTC) #30
esprehn
On 2015/12/12 at 01:49:52, esprehn wrote: > https://codereview.chromium.org/1449623002/diff/160001/third_party/WebKit/Source/core/dom/IntersectionObserver.h > File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right): > > https://codereview.chromium.org/1449623002/diff/160001/third_party/WebKit/Source/core/dom/IntersectionObserver.h#newcode62 ...
5 years ago (2015-12-12 01:57:02 UTC) #31
szager1
All comments addressed, and the tests are passing again. https://codereview.chromium.org/1449623002/diff/100001/third_party/WebKit/Source/core/dom/Element.h File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/1449623002/diff/100001/third_party/WebKit/Source/core/dom/Element.h#newcode547 third_party/WebKit/Source/core/dom/Element.h:547: ...
5 years ago (2015-12-16 19:15:37 UTC) #35
esprehn
I think there's some confusion here, if passing no root is the same as passing ...
5 years ago (2015-12-16 20:58:32 UTC) #36
szager1
On 2015/12/16 20:58:32, esprehn wrote: > I think there's some confusion here, if passing no ...
5 years ago (2015-12-16 21:42:26 UTC) #37
esprehn
On 2015/12/16 at 21:42:26, szager wrote: > On 2015/12/16 20:58:32, esprehn wrote: > > I ...
5 years ago (2015-12-16 22:12:01 UTC) #38
szager1
Uploaded one more patch set to address lifetime issues since the code straddles the ref-counted ...
5 years ago (2015-12-17 00:52:02 UTC) #39
szager1
On 2015/12/16 22:12:01, esprehn wrote: > On 2015/12/16 at 21:42:26, szager wrote: > > On ...
5 years ago (2015-12-17 00:57:46 UTC) #41
dcheng
Did you ever get a chance to explore removing the usage of WeakPtrFactory, or is ...
5 years ago (2015-12-17 01:00:15 UTC) #42
szager1
On 2015/12/17 01:00:15, dcheng wrote: > Did you ever get a chance to explore removing ...
5 years ago (2015-12-17 01:04:45 UTC) #43
esprehn
https://codereview.chromium.org/1449623002/diff/200001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1449623002/diff/200001/third_party/WebKit/Source/core/dom/Document.cpp#newcode4986 third_party/WebKit/Source/core/dom/Document.cpp:4986: IntersectionObserverController* Document::intersectionObserverController() We usually make these return a reference ...
5 years ago (2015-12-17 01:40:29 UTC) #44
szager1
https://codereview.chromium.org/1449623002/diff/200001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1449623002/diff/200001/third_party/WebKit/Source/core/dom/Document.cpp#newcode4986 third_party/WebKit/Source/core/dom/Document.cpp:4986: IntersectionObserverController* Document::intersectionObserverController() On 2015/12/17 01:40:28, esprehn wrote: > We ...
5 years ago (2015-12-17 20:27:26 UTC) #45
szager1
ping
5 years ago (2015-12-21 18:01:44 UTC) #46
haraken
This is an amazingly complicated but exciting CL :) It looks like we need a ...
5 years ago (2015-12-22 01:13:30 UTC) #47
haraken
I'd suggest writing a diagram of the lifetime relationship of the following objects (in both ...
5 years ago (2015-12-22 01:17:31 UTC) #48
esprehn
This patch is big, I might have missed some things. I think you and haraken@ ...
5 years ago (2015-12-22 07:50:33 UTC) #49
szager1
https://codereview.chromium.org/1449623002/diff/240001/third_party/WebKit/Source/bindings/core/v8/V8IntersectionObserverCallback.h File third_party/WebKit/Source/bindings/core/v8/V8IntersectionObserverCallback.h (right): https://codereview.chromium.org/1449623002/diff/240001/third_party/WebKit/Source/bindings/core/v8/V8IntersectionObserverCallback.h#newcode24 third_party/WebKit/Source/bindings/core/v8/V8IntersectionObserverCallback.h:24: void handleEvent(HeapVector<Member<IntersectionObserverEntry>>&, IntersectionObserver&) override; On 2015/12/22 01:13:29, haraken wrote: ...
5 years ago (2015-12-22 07:53:42 UTC) #50
haraken
Thanks for the update :) I'll be running into a vacation, so let's chat about ...
5 years ago (2015-12-22 08:10:50 UTC) #51
szager1
I should be up-to-date with the latest comments from esprehn and haraken. https://codereview.chromium.org/1449623002/diff/240001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp File third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp ...
5 years ago (2015-12-22 08:49:50 UTC) #52
szager1
https://codereview.chromium.org/1449623002/diff/240001/third_party/WebKit/Source/core/dom/IntersectionObserver.cpp File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/1449623002/diff/240001/third_party/WebKit/Source/core/dom/IntersectionObserver.cpp#newcode218 third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:218: m_observations.remove(&observation); On 2015/12/22 01:13:30, haraken wrote: > > Add ...
5 years ago (2015-12-22 19:04:36 UTC) #53
haraken
Thanks for the update! Let me ask a couple of questions about the design in ...
4 years, 12 months ago (2015-12-28 05:54:55 UTC) #54
MikeB
On 2015/12/28 05:54:55, haraken wrote: > > - This is a question against the spec, ...
4 years, 12 months ago (2015-12-28 18:26:47 UTC) #55
szager1
On 2015/12/28 05:54:55, haraken wrote: > Thanks for the update! > > Let me ask ...
4 years, 12 months ago (2015-12-28 19:06:20 UTC) #56
haraken
Thanks for the clarification! On 2015/12/28 19:06:20, szager1 wrote: > On 2015/12/28 05:54:55, haraken wrote: ...
4 years, 11 months ago (2015-12-29 02:39:13 UTC) #57
szager1
On 2015/12/29 02:39:13, haraken wrote: > > > - What do you mean by "Callback ...
4 years, 11 months ago (2015-12-29 10:37:12 UTC) #58
szager1
Anyone available and willing to lgtm? We are very far behind schedule in getting this ...
4 years, 11 months ago (2015-12-29 20:06:50 UTC) #59
ojan
Please see my earlier comment: Please move the rootMargin code to a followup patch. This ...
4 years, 11 months ago (2015-12-29 21:12:45 UTC) #60
haraken
On 2015/12/29 21:12:45, ojan wrote: > Please see my earlier comment: Please move the rootMargin ...
4 years, 11 months ago (2015-12-30 00:02:28 UTC) #61
haraken
(Since I'm now OOO, the best thing I can do is to post comments in ...
4 years, 11 months ago (2015-12-30 00:57:38 UTC) #62
sof
dbc trifles + looks like a rebase is in order. https://codereview.chromium.org/1449623002/diff/300001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.h File third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.h (right): https://codereview.chromium.org/1449623002/diff/300001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.h#newcode47 ...
4 years, 11 months ago (2015-12-30 13:09:33 UTC) #63
szager1
On 2015/12/30 00:57:38, haraken wrote: > Thanks, now I understand what the tracking document is. ...
4 years, 11 months ago (2015-12-30 18:43:05 UTC) #64
haraken
On 2015/12/30 18:43:05, szager1 wrote: > On 2015/12/30 00:57:38, haraken wrote: > > > Thanks, ...
4 years, 11 months ago (2016-01-01 02:26:15 UTC) #65
szager1
On 2016/01/01 02:26:15, haraken wrote: > On 2015/12/30 18:43:05, szager1 wrote: > > > > ...
4 years, 11 months ago (2016-01-01 04:10:06 UTC) #66
haraken
I think I now understand the lifetime relationship. The main points of my comments are ...
4 years, 11 months ago (2016-01-02 13:47:41 UTC) #67
szager1
https://codereview.chromium.org/1449623002/diff/300001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.h File third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.h (right): https://codereview.chromium.org/1449623002/diff/300001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.h#newcode47 third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.h:47: } // namespace blink { On 2015/12/30 13:09:33, sof ...
4 years, 11 months ago (2016-01-02 18:05:59 UTC) #68
szager1
Here is the teardown scenario that led me to add dispose() and disconnect() and checkRootAndDetachIfNecessary() ...
4 years, 11 months ago (2016-01-02 19:18:34 UTC) #69
haraken
On 2016/01/02 19:18:34, szager1 wrote: > Here is the teardown scenario that led me to ...
4 years, 11 months ago (2016-01-03 16:34:20 UTC) #70
szager1
I split out the suspend/resume and root margin features into separate patches. Here's the full ...
4 years, 11 months ago (2016-01-04 03:53:04 UTC) #71
szager1
Latest patch simplifies the teardown logic. PTAL, no bystanders please. I'm about ready to stab ...
4 years, 11 months ago (2016-01-06 22:33:50 UTC) #72
haraken
Looks much simpler. Here is the final round of comments. https://codereview.chromium.org/1449623002/diff/400001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp File third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp (right): https://codereview.chromium.org/1449623002/diff/400001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp#newcode31 ...
4 years, 11 months ago (2016-01-07 00:48:17 UTC) #73
szager1
https://codereview.chromium.org/1449623002/diff/400001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp File third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp (right): https://codereview.chromium.org/1449623002/diff/400001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp#newcode31 third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp:31: HeapHashMap<Member<IntersectionObserver>, Member<IntersectionObservation>>::iterator i = m_intersectionObservations.find(&observer); On 2016/01/07 00:48:17, haraken ...
4 years, 11 months ago (2016-01-07 07:12:22 UTC) #74
haraken
bindings/, oilpan and the object lifetime relationships LGTM.
4 years, 11 months ago (2016-01-07 07:22:15 UTC) #75
haraken
https://codereview.chromium.org/1449623002/diff/400001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp File third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp (right): https://codereview.chromium.org/1449623002/diff/400001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp#newcode79 third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp:79: observer->disconnect(); On 2016/01/07 07:12:22, szager1 wrote: > On 2016/01/07 ...
4 years, 11 months ago (2016-01-07 07:38:13 UTC) #76
szager1
On 2016/01/07 07:38:13, haraken wrote: > https://codereview.chromium.org/1449623002/diff/400001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp > File third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp > (right): > > https://codereview.chromium.org/1449623002/diff/400001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp#newcode79 ...
4 years, 11 months ago (2016-01-07 09:56:21 UTC) #77
haraken
On 2016/01/07 09:56:21, szager1 wrote: > On 2016/01/07 07:38:13, haraken wrote: > > > https://codereview.chromium.org/1449623002/diff/400001/third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.cpp ...
4 years, 11 months ago (2016-01-07 10:50:23 UTC) #78
szager1
On 2016/01/07 10:50:23, haraken wrote: > > Hmm. Observation::disconnect is doing the following three things: ...
4 years, 11 months ago (2016-01-07 19:12:13 UTC) #79
szager1
On 2016/01/07 19:12:13, szager1 wrote: > > 3.2) Post-oilpan: this happens during gc, during a ...
4 years, 11 months ago (2016-01-07 20:27:37 UTC) #80
szager1
On 2016/01/07 20:27:37, szager1 wrote: > On 2016/01/07 19:12:13, szager1 wrote: > > > > ...
4 years, 11 months ago (2016-01-07 20:38:44 UTC) #81
eae
> I would also like to suggest that any additional work on the teardown code ...
4 years, 11 months ago (2016-01-07 20:40:00 UTC) #82
haraken
On 2016/01/07 20:40:00, eae wrote: > > I would also like to suggest that any ...
4 years, 11 months ago (2016-01-07 23:40:05 UTC) #83
szager1
It would be nice to hear from esprehn and/or ojan, but if I don't, then ...
4 years, 11 months ago (2016-01-08 00:38:55 UTC) #84
haraken
Overall, I'm okay with landing this CL with some incorrect tear-down logic in oilpan (we ...
4 years, 11 months ago (2016-01-08 00:54:30 UTC) #85
szager1
On 2016/01/08 00:54:30, haraken wrote: > > If we remove observation->disconnect() from > ElementIntersectionObserverData::dispose() in ...
4 years, 11 months ago (2016-01-08 01:16:14 UTC) #86
haraken
On 2016/01/08 01:16:14, szager1 wrote: > On 2016/01/08 00:54:30, haraken wrote: > > > > ...
4 years, 11 months ago (2016-01-08 02:26:25 UTC) #87
haraken
On 2016/01/08 01:16:14, szager1 wrote: > On 2016/01/08 00:54:30, haraken wrote: > > > > ...
4 years, 11 months ago (2016-01-08 02:39:25 UTC) #88
szager1
On 2016/01/08 02:26:25, haraken wrote: > On 2016/01/08 01:16:14, szager1 wrote: > > On 2016/01/08 ...
4 years, 11 months ago (2016-01-08 02:39:31 UTC) #89
szager1
On 2016/01/08 02:39:25, haraken wrote: > On 2016/01/08 01:16:14, szager1 wrote: > > On 2016/01/08 ...
4 years, 11 months ago (2016-01-08 02:40:37 UTC) #90
haraken
On 2016/01/08 02:40:37, szager1 wrote: > On 2016/01/08 02:39:25, haraken wrote: > > On 2016/01/08 ...
4 years, 11 months ago (2016-01-08 02:42:12 UTC) #91
haraken
On 2016/01/08 02:39:31, szager1 wrote: > On 2016/01/08 02:26:25, haraken wrote: > > On 2016/01/08 ...
4 years, 11 months ago (2016-01-08 02:47:06 UTC) #92
dcheng
Noticed this while skimming through the latest PS. https://codereview.chromium.org/1449623002/diff/440001/third_party/WebKit/Source/core/dom/IntersectionObservation.h File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right): https://codereview.chromium.org/1449623002/diff/440001/third_party/WebKit/Source/core/dom/IntersectionObservation.h#newcode9 third_party/WebKit/Source/core/dom/IntersectionObservation.h:9: #define ...
4 years, 11 months ago (2016-01-08 03:02:04 UTC) #93
szager1
https://codereview.chromium.org/1449623002/diff/440001/third_party/WebKit/Source/core/dom/IntersectionObservation.h File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right): https://codereview.chromium.org/1449623002/diff/440001/third_party/WebKit/Source/core/dom/IntersectionObservation.h#newcode9 third_party/WebKit/Source/core/dom/IntersectionObservation.h:9: #define IntersectionObservation_h On 2016/01/08 03:02:04, dcheng wrote: > The ...
4 years, 11 months ago (2016-01-08 05:18:46 UTC) #94
eae
LGTM
4 years, 11 months ago (2016-01-08 17:58:37 UTC) #95
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1449623002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1449623002/500001
4 years, 11 months ago (2016-01-08 22:09:40 UTC) #98
commit-bot: I haz the power
Committed patchset #26 (id:500001)
4 years, 11 months ago (2016-01-08 23:48:02 UTC) #100
commit-bot: I haz the power
Patchset 26 (id:??) landed as https://crrev.com/b5cff0041ec225f4eaac870e87d6189321754983 Cr-Commit-Position: refs/heads/master@{#368458}
4 years, 11 months ago (2016-01-08 23:49:25 UTC) #102
haraken
I don't think the tear-down logic for non-oilpan in PS26 is nice. Please address the ...
4 years, 11 months ago (2016-01-09 04:11:54 UTC) #103
szager1
On 2016/01/09 04:11:54, haraken wrote: > I don't think the tear-down logic for non-oilpan in ...
4 years, 11 months ago (2016-01-10 03:38:54 UTC) #104
haraken
https://codereview.chromium.org/1449623002/diff/500001/third_party/WebKit/Source/core/dom/IntersectionObservation.cpp File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right): https://codereview.chromium.org/1449623002/diff/500001/third_party/WebKit/Source/core/dom/IntersectionObservation.cpp#newcode80 third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:80: LayoutObject* targetLayoutObject = m_target->layoutObject(); Given that m_target is a ...
4 years, 11 months ago (2016-01-10 12:22:28 UTC) #105
szager1
4 years, 11 months ago (2016-01-10 17:18:26 UTC) #106
Message was sent while issue was closed.
https://codereview.chromium.org/1449623002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/1449623002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:80: LayoutObject*
targetLayoutObject = m_target->layoutObject();
On 2016/01/10 12:22:28, haraken wrote:
> 
> Given that m_target is a WeakMember, it can be cleared anytime. You need to
have
> an if(!m_target) check here and all other places that use m_target.
> 
> As I mentioned before, the programming rule is:
> 
> - If A may outlive B and you want to have a pointer from A to B, you should
use
> a WeakPtr (in non-oilpan) or a WeakMember (in oilpan).
> 
> - You must have a nullptr check before using the WeakPtr/WeakMember.
> 
> This CL violates the rule in multiple places, so please fix them in a
follow-up.

All of the un-checked uses of m_target are in private methods that are only
called through computeIntersectionObservations, which has a bool check for
m_target up front.  I will add ASSERT's.

Powered by Google App Engine
This is Rietveld 408576698