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

Issue 1287623002: Delete blink code for scroll-blocks-on (Closed)

Created:
5 years, 4 months ago by Rick Byers
Modified:
5 years, 4 months ago
Reviewers:
Ian Vollick, dtapuska
CC:
blink-reviews, vivekg_samsung, dshwang, eae+blinkwatch, vivekg, apavlov+blink_chromium.org, rwlbuis, caseq+blink_chromium.org, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-css, szager+layoutwatch_chromium.org, yurys+blink_chromium.org, Justin Novosad, danakj, blink-reviews-dom_chromium.org, dglazkov+blink, Rik, jchaffraix+rendering, devtools-reviews_chromium.org, blink-reviews-style_chromium.org, zoltan1, sof, jbroman, lushnikov+blink_chromium.org, krit, darktears, Stephen Chennney, blink-reviews-rendering, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, blink-layers+watch_chromium.org, pfeldman+blink_chromium.org, f(malita), Inactive, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org, caseq, skobes, Timothy Loh
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Delete blink code for scroll-blocks-on We've decided that controlling scroll blocking behavior is something that really needs to be tied to event listeners via a JS API, not a CSS property. The platform needs to mediate the composition here (eg. for two unrelated libraries both adding touch handlers to the document), and there's no good way to mediate that composition using CSS. It's really a property of the event handler and so should be specified at addEventListener time. http://crbug.com/489802 tracks the first step of that design (EventListenerOptions). So it's time to remove the scroll-blocks-on CSS support from blink. I expect the utlimate compositor integration for EventListenerOptions mayCancel to be similar, and so for now I'm leaving the hooks in place in GraphicsLayer. This change also fixes a regression where iframes could occasionally not get displayed. I don't understand how the scroll-blocks-on code (even when not enabled) was triggering that problem, but since it's going away I'm not going to investigate further. BUG=347272, 490358 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200448

Patch Set 1 #

Total comments: 2

Patch Set 2 : dtapsuka CR feedback #

Patch Set 3 : Merge with trunk (no changes from previous patch) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+56 lines, -1038 lines) Patch
D LayoutTests/compositing/layer-creation/scroll-blocks-on-default.html View 1 chunk +0 lines, -20 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-default-expected.txt View 1 chunk +0 lines, -23 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-document.html View 1 chunk +0 lines, -14 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-document-expected.txt View 1 chunk +0 lines, -17 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-explicit.html View 1 chunk +0 lines, -26 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-explicit-expected.txt View 1 chunk +0 lines, -34 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-iframe.html View 1 chunk +0 lines, -38 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-iframe-expected.txt View 1 chunk +0 lines, -122 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-script.html View 1 chunk +0 lines, -19 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-script-expected.txt View 1 chunk +0 lines, -26 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-svg.svg View 1 chunk +0 lines, -18 lines 0 comments Download
D LayoutTests/compositing/layer-creation/scroll-blocks-on-svg-expected.txt View 1 chunk +0 lines, -17 lines 0 comments Download
D LayoutTests/compositing/squashing/no-squashing-for-scroll-blocks-on.html View 1 chunk +0 lines, -19 lines 0 comments Download
D LayoutTests/compositing/squashing/no-squashing-for-scroll-blocks-on-expected.txt View 1 chunk +0 lines, -43 lines 0 comments Download
M LayoutTests/fast/css/getComputedStyle/computed-style-listing-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/fast/css/getComputedStyle/computed-style-without-renderer-listing-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
D LayoutTests/fast/css/scroll-blocks-on-parsing.html View 1 chunk +0 lines, -42 lines 0 comments Download
D LayoutTests/fast/css/scroll-blocks-on-parsing-expected.txt View 1 chunk +0 lines, -57 lines 0 comments Download
M LayoutTests/inspector/elements/styles-4/styles-new-API-expected.txt View 1 chunk +0 lines, -3 lines 0 comments Download
D LayoutTests/platform/android/compositing/layer-creation/scroll-blocks-on-default-expected.txt View 1 chunk +0 lines, -20 lines 0 comments Download
D LayoutTests/platform/android/compositing/layer-creation/scroll-blocks-on-document-expected.txt View 1 chunk +0 lines, -14 lines 0 comments Download
D LayoutTests/platform/android/compositing/layer-creation/scroll-blocks-on-explicit-expected.txt View 1 chunk +0 lines, -26 lines 0 comments Download
D LayoutTests/platform/android/compositing/layer-creation/scroll-blocks-on-iframe-expected.txt View 1 chunk +0 lines, -108 lines 0 comments Download
D LayoutTests/platform/android/compositing/layer-creation/scroll-blocks-on-script-expected.txt View 1 chunk +0 lines, -21 lines 0 comments Download
D LayoutTests/platform/android/compositing/layer-creation/scroll-blocks-on-svg-expected.txt View 1 chunk +0 lines, -14 lines 0 comments Download
D LayoutTests/platform/android/compositing/squashing/no-squashing-for-scroll-blocks-on-expected.txt View 1 chunk +0 lines, -38 lines 0 comments Download
M LayoutTests/svg/css/getComputedStyle-listing-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/webexposed/css-properties-as-js-properties-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/css/CSSComputedStyleDeclaration.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/css/CSSPrimitiveValueMappings.h View 2 chunks +0 lines, -20 lines 0 comments Download
M Source/core/css/CSSProperties.in View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/css/CSSValueKeywords.in View 1 chunk +0 lines, -6 lines 0 comments Download
M Source/core/css/ComputedStyleCSSValueMapping.cpp View 2 chunks +0 lines, -19 lines 0 comments Download
M Source/core/css/html.css View 1 chunk +0 lines, -4 lines 0 comments Download
M Source/core/css/parser/CSSPropertyParser.h View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/css/parser/CSSPropertyParser.cpp View 1 2 2 chunks +0 lines, -32 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/css/svg.css View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 chunks +1 line, -5 lines 0 comments Download
M Source/core/frame/UseCounter.cpp View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/layout/LayoutBox.cpp View 1 2 1 chunk +0 lines, -9 lines 0 comments Download
M Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.h View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp View 3 chunks +0 lines, -16 lines 0 comments Download
M Source/core/layout/compositing/CompositingReasonFinder.h View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/layout/compositing/CompositingReasonFinder.cpp View 3 chunks +0 lines, -53 lines 0 comments Download
M Source/core/style/ComputedStyle.h View 4 chunks +0 lines, -5 lines 0 comments Download
M Source/core/style/StyleRareNonInheritedData.h View 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/style/StyleRareNonInheritedData.cpp View 3 chunks +1 line, -4 lines 0 comments Download
M Source/core/testing/Internals.idl View 1 2 1 chunk +2 lines, -3 lines 0 comments Download
M Source/devtools/front_end/timeline/LayerDetailsView.js View 1 chunk +0 lines, -1 line 0 comments Download
M Source/platform/RuntimeEnabledFeatures.in View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M Source/platform/graphics/CompositingReasons.h View 1 3 chunks +47 lines, -50 lines 0 comments Download
M Source/platform/graphics/CompositingReasons.cpp View 1 chunk +0 lines, -3 lines 0 comments Download
M Source/platform/graphics/GraphicsLayer.cpp View 2 chunks +3 lines, -11 lines 0 comments Download
M Source/platform/graphics/GraphicsLayerClient.h View 1 chunk +2 lines, -3 lines 0 comments Download

Messages

Total messages: 15 (5 generated)
Rick Byers
dtapuska@ please take a look. This is a fairly mechanical removal of code I (sadly) ...
5 years, 4 months ago (2015-08-11 17:02:59 UTC) #2
dtapuska
https://codereview.chromium.org/1287623002/diff/1/Source/platform/graphics/CompositingReasons.h File Source/platform/graphics/CompositingReasons.h (left): https://codereview.chromium.org/1287623002/diff/1/Source/platform/graphics/CompositingReasons.h#oldcode32 Source/platform/graphics/CompositingReasons.h:32: const uint64_t CompositingReasonScrollBlocksOn = UINT64_C(1) << 14; When reasons ...
5 years, 4 months ago (2015-08-11 17:15:11 UTC) #3
Rick Byers
https://codereview.chromium.org/1287623002/diff/1/Source/platform/graphics/CompositingReasons.h File Source/platform/graphics/CompositingReasons.h (left): https://codereview.chromium.org/1287623002/diff/1/Source/platform/graphics/CompositingReasons.h#oldcode32 Source/platform/graphics/CompositingReasons.h:32: const uint64_t CompositingReasonScrollBlocksOn = UINT64_C(1) << 14; On 2015/08/11 ...
5 years, 4 months ago (2015-08-12 02:20:29 UTC) #4
dtapuska
On 2015/08/12 02:20:29, Rick Byers wrote: > https://codereview.chromium.org/1287623002/diff/1/Source/platform/graphics/CompositingReasons.h > File Source/platform/graphics/CompositingReasons.h (left): > > https://codereview.chromium.org/1287623002/diff/1/Source/platform/graphics/CompositingReasons.h#oldcode32 ...
5 years, 4 months ago (2015-08-12 14:23:10 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287623002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287623002/40001
5 years, 4 months ago (2015-08-12 22:24:01 UTC) #7
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an ...
5 years, 4 months ago (2015-08-12 22:24:05 UTC) #9
Rick Byers
Dave is actually a (recently) approved committer. But it doesn't hurt to have a Source/Platform ...
5 years, 4 months ago (2015-08-12 22:29:23 UTC) #11
Ian Vollick
On 2015/08/12 22:29:23, Rick Byers wrote: > Dave is actually a (recently) approved committer. But ...
5 years, 4 months ago (2015-08-12 23:45:54 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287623002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287623002/40001
5 years, 4 months ago (2015-08-12 23:46:36 UTC) #14
commit-bot: I haz the power
5 years, 4 months ago (2015-08-13 02:09:55 UTC) #15
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200448

Powered by Google App Engine
This is Rietveld 408576698