Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 2881423004: Stop retrieving scrolling element id from the CompositorElementId. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months ago by chrishtr
Modified:
5 months ago
Reviewers:
pdr.
CC:
blink-reviews, chromium-reviews, kenneth.christiansen
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Stop retrieving scrolling element id from the CompositorElementId. Instead, store it locally. This is possible because ScrollState is the only way to set current_native_scrolling_element on cc::ScrollStateData. Furthermore, ScrollState is a garbage-collected, script-observable class, so referencing Element from it is safe. Also note that ScrollState is part of an unlaunched feature. BUG=718564 Review-Url: https://codereview.chromium.org/2881423004 Cr-Commit-Position: refs/heads/master@{#472826} Committed: https://chromium.googlesource.com/chromium/src/+/1713618a7ed0036cb64f87a096a94f43a6a8761e

Patch Set 1 #

Patch Set 2 : none #

Patch Set 3 : none #

Total comments: 1

Patch Set 4 : none #

Total comments: 1

Patch Set 5 : Merge branch 'master' into scrollstate #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -19 lines) Patch
M third_party/WebKit/Source/core/page/scrolling/ScrollState.h View 4 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp View 1 2 1 chunk +6 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/CompositorElementId.h View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp View 1 1 chunk +0 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/CompositorElementIdTest.cpp View 1 1 chunk +4 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 36 (23 generated)
chrishtr
https://codereview.chromium.org/2881423004/diff/40001/third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp File third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp (right): https://codereview.chromium.org/2881423004/diff/40001/third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp#newcode96 third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp:96: if (data_->current_native_scrolling_element() == CompositorElementId()) { The compositor may clear ...
5 months ago (2017-05-16 23:46:45 UTC) #3
chrishtr
This CL will allow us to remove the use of DOMNodeIds in a followup, which ...
5 months ago (2017-05-16 23:47:18 UTC) #5
pdr.
https://codereview.chromium.org/2881423004/diff/60001/third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp File third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp (right): https://codereview.chromium.org/2881423004/diff/60001/third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp#newcode97 third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp:97: element_.Clear(); Instead of clearing element_ in the getter, can ...
5 months ago (2017-05-17 00:24:19 UTC) #7
chrishtr
On 2017/05/17 at 00:24:19, pdr wrote: > https://codereview.chromium.org/2881423004/diff/60001/third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp > File third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp (right): > > https://codereview.chromium.org/2881423004/diff/60001/third_party/WebKit/Source/core/page/scrolling/ScrollState.cpp#newcode97 ...
5 months ago (2017-05-17 00:36:10 UTC) #8
pdr.
On 2017/05/17 at 00:36:10, chrishtr wrote: > On 2017/05/17 at 00:24:19, pdr wrote: > > ...
5 months ago (2017-05-17 00:51:36 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2881423004/60001
5 months ago (2017-05-17 03:04:46 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/456897)
5 months ago (2017-05-17 04:38:36 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2881423004/60001
5 months ago (2017-05-17 16:53:43 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/457712)
5 months ago (2017-05-17 19:43:41 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2881423004/80001
5 months ago (2017-05-18 01:18:40 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/217889)
5 months ago (2017-05-18 01:25:33 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2881423004/80001
5 months ago (2017-05-18 14:56:28 UTC) #33
commit-bot: I haz the power
5 months ago (2017-05-18 15:46:58 UTC) #36
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/1713618a7ed0036cb64f87a096a9...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa