Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1988633002:
Schedule a frame from IntersectionObserver::observe. (Closed)
|
Created:
4 years, 7 months ago by szager1 Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, philipwalton_google.com, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSchedule a frame from IntersectionObserver::observe.
If the target is intersecting the root when observe() is called, a
notification should be queued up immediately, but if no frame is
scheduled, then the intersection algorithm will not run.
BUG=612323
R=ojan@chromium.org,eae@chromium.org
Committed: https://crrev.com/9d6cc87951566e8fc9be1a04fefb2054f870ab1e
Cr-Commit-Position: refs/heads/master@{#395460}
Patch Set 1 #Patch Set 2 : Check for non-null rootFrame #Patch Set 3 : test #
Total comments: 8
Patch Set 4 : nits #Patch Set 5 : test fixes #
Total comments: 2
Patch Set 6 : Oilpan doesn't like stack-allocated callback #
Messages
Total messages: 35 (16 generated)
Description was changed from ========== Schedule a frame from IntersectionObserver::observe. If the target is intersecting the root when observe() is called, a notification should be queued up immediately, but if no frame is scheduled, then the intersection algorithm will not run. BUG=612323 R=ojan@chromium.org,eae@chromium.org ========== to ========== Schedule a frame from IntersectionObserver::observe. If the target is intersecting the root when observe() is called, a notification should be queued up immediately, but if no frame is scheduled, then the intersection algorithm will not run. BUG=612323 R=ojan@chromium.org,eae@chromium.org ==========
Burning the midnight oil I see, don't work too hard! LGTM
The CQ bit was checked by eae@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988633002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988633002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1988633002/#ps20001 (title: "Check for non-null rootFrame")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988633002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988633002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Test?
The CQ bit was checked by szager@chromium.org
The CQ bit was unchecked by szager@chromium.org
szager@chromium.org changed reviewers: + esprehn@chromium.org
On 2016/05/18 21:27:26, ojan wrote: > Test? Added IntersectionObserverTest.cpp. +esprehn for Source/web/OWNERS
https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp (right): https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp:17: using testing::_; I don't think you use this? https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp:57: webView().updateAllLifecyclePhases(); remove this, beginFrame does it https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp:58: testing::runPendingTasks(); Why do you need to run tasks?
https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp (right): https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp:17: using testing::_; On 2016/05/19 22:55:59, esprehn wrote: > I don't think you use this? Removed. https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp:57: webView().updateAllLifecyclePhases(); On 2016/05/19 22:55:59, esprehn wrote: > remove this, beginFrame does it Done. https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp:58: testing::runPendingTasks(); On 2016/05/19 22:55:59, esprehn wrote: > Why do you need to run tasks? beginFrame() will schedule a task to generate a frame, but the test needs to make sure a frame was generated and the document is compositing-clean at this point. I cargo-culted this from FrameThrottlingTest::compositeFrame(), and it works as expected.
https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp (right): https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp:58: testing::runPendingTasks(); On 2016/05/19 at 23:58:40, szager1 wrote: > On 2016/05/19 22:55:59, esprehn wrote: > > Why do you need to run tasks? > > beginFrame() will schedule a task to generate a frame, but the test needs to make sure a frame was generated and the document is compositing-clean at this point. I cargo-culted this from FrameThrottlingTest::compositeFrame(), and it works as expected. beginFrame() doesn't schedule a task to generate a frame, it does it right then. The FrameThrottlingTest is a special case where they want to pump tasks since one of those tasks will toggle the throttling behavior async. Most tests don't need to do that, you shouldn't need to either.
https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp (right): https://codereview.chromium.org/1988633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp:58: testing::runPendingTasks(); On 2016/05/20 00:38:34, esprehn wrote: > On 2016/05/19 at 23:58:40, szager1 wrote: > > On 2016/05/19 22:55:59, esprehn wrote: > > > Why do you need to run tasks? > > > > beginFrame() will schedule a task to generate a frame, but the test needs to > make sure a frame was generated and the document is compositing-clean at this > point. I cargo-culted this from FrameThrottlingTest::compositeFrame(), and it > works as expected. > > beginFrame() doesn't schedule a task to generate a frame, it does it right then. > The FrameThrottlingTest is a special case where they want to pump tasks since > one of those tasks will toggle the throttling behavior async. Most tests don't > need to do that, you shouldn't need to either. Indeed, I see you're right; I removed the call to runPendingTasks.
lgtm https://codereview.chromium.org/1988633002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/1988633002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:190: rootFrameView->scheduleAnimation(); I think this is probably broken for OOPIF, we need to audit the IO code in general for how to make it work in OOPIF.
https://codereview.chromium.org/1988633002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/1988633002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:190: rootFrameView->scheduleAnimation(); On 2016/05/20 22:40:52, esprehn wrote: > I think this is probably broken for OOPIF, we need to audit the IO code in > general for how to make it work in OOPIF. Oh yes, undoubtedly, IO is completely broken for OOPIF.
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1988633002/#ps80001 (title: "test fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988633002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988633002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1988633002/#ps100001 (title: "Oilpan doesn't like stack-allocated callback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988633002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988633002/100001
Message was sent while issue was closed.
Description was changed from ========== Schedule a frame from IntersectionObserver::observe. If the target is intersecting the root when observe() is called, a notification should be queued up immediately, but if no frame is scheduled, then the intersection algorithm will not run. BUG=612323 R=ojan@chromium.org,eae@chromium.org ========== to ========== Schedule a frame from IntersectionObserver::observe. If the target is intersecting the root when observe() is called, a notification should be queued up immediately, but if no frame is scheduled, then the intersection algorithm will not run. BUG=612323 R=ojan@chromium.org,eae@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Schedule a frame from IntersectionObserver::observe. If the target is intersecting the root when observe() is called, a notification should be queued up immediately, but if no frame is scheduled, then the intersection algorithm will not run. BUG=612323 R=ojan@chromium.org,eae@chromium.org ========== to ========== Schedule a frame from IntersectionObserver::observe. If the target is intersecting the root when observe() is called, a notification should be queued up immediately, but if no frame is scheduled, then the intersection algorithm will not run. BUG=612323 R=ojan@chromium.org,eae@chromium.org Committed: https://crrev.com/9d6cc87951566e8fc9be1a04fefb2054f870ab1e Cr-Commit-Position: refs/heads/master@{#395460} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9d6cc87951566e8fc9be1a04fefb2054f870ab1e Cr-Commit-Position: refs/heads/master@{#395460} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
