Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2600593002:
Fix touch event flag may not be updated correctly by transform which causes no layout (Closed)
DescriptionFix touch event flag may not be updated correctly by transform which causes no layout
BUG=673102
TEST=third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2600593002
Cr-Original-Commit-Position: refs/heads/master@{#443580}
Committed: https://chromium.googlesource.com/chromium/src/+/3ad9090bc969c9e2b450ec249d4dfac398bb27b8
Review-Url: https://codereview.chromium.org/2600593002
Cr-Commit-Position: refs/heads/master@{#443921}
Committed: https://chromium.googlesource.com/chromium/src/+/48e70529c038713fe0b2ddd21d555a2791f95071
Patch Set 1 #Patch Set 2 : Use absolute position for cross-platform purpose #
Total comments: 22
Patch Set 3 : Update the logic to focus on non-passive touchEvents #
Total comments: 6
Patch Set 4 : Refactoring #Patch Set 5 : Bug fixed #
Total comments: 6
Patch Set 6 : Optimization && nit #
Total comments: 2
Patch Set 7 : spv2 failure added in FlagExpectations #
Messages
Total messages: 38 (14 generated)
Description was changed from ========== Fix touch event flag may not be updated correctly by transform which causes no layout BUG=673102 TEST=third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html ========== to ========== Fix touch event flag may not be updated correctly by transform which causes no layout BUG=673102 TEST=third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
yigu@chromium.org changed reviewers: + majidvp@chromium.org
Hi Majid, please take a look at the fix. Thanks! Yi
https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html (right): https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:15: a slider transfroms without creating layout. s/creating/causing/ https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:15: a slider transfroms without creating layout. what do you mean by a slider? This is true for any element and not just a "slider". https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:19: <div id='console' style='display:none;'></div> The use of single quote (') and double quote (") is inconsistent here and elsewhere in this test. Please make it consistent. https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:22: <script src="../../../../resources/testharnessreport.js"></script> testharness.js and testharnessreport.js seem unused. Please remove. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:25: window.onload = () => { do you actually need to have the test in onload handler? https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:29: logRects(box, true); 'compositor-touch-hit-rects.js' already has facilities for what you are doing here i.e., adding touchstart handler, forcing compositing, logging the rect). So I think you could just replace this test with a simple 'preRunHandlerForTest' That updates the transform. See for example: fast/events/touch/compositor-touch-hit-rects-scroll.html https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:435: Invalidating touch regions on every transform change for every node feels like using a BIG hammer. Is it possible to limit doing this only for certain transform changes e.g., perhaps only when the element actually has a 'touchstart' handler? https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:437: // should be called to correctly update the touch event flags The comment seems superfluous. It is also a bit incorrect because the logic is calling notifyGeometryChanged() regardless of whether we need a layout or not. I think you should just remove the comment. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:438: if (hasTransform) { hmmm, I think this check here is incorrect. You want to recompute on transform change whether it is adding a transform or removing one. In fact, why don't you add a test case where removing the transform causes the touch target to be recomputed. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:441: scrollingCoordinator->notifyGeometryChanged(); |notifyGeometryChanged| causes recomputation of not only touch regions but also slow scroll regions. Is this what you wanted? https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:443: I rather move this logic at the bottom of the function once transform update is complete i.e., next to where we notify FrameView of geometry changes.
Hi Majid, Thanks for the feedback! Changes have been made accordingly. PTAL. Yi https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html (right): https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:15: a slider transfroms without creating layout. On 2017/01/02 21:01:10, majidvp wrote: > s/creating/causing/ Done. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:15: a slider transfroms without creating layout. On 2017/01/02 21:01:10, majidvp wrote: > s/creating/causing/ Done. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:19: <div id='console' style='display:none;'></div> On 2017/01/02 21:01:10, majidvp wrote: > The use of single quote (') and double quote (") is inconsistent here and > elsewhere in this test. Please make it consistent. > > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... Done. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:22: <script src="../../../../resources/testharnessreport.js"></script> On 2017/01/02 21:01:10, majidvp wrote: > testharness.js and testharnessreport.js seem unused. Please remove. Done. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:25: window.onload = () => { On 2017/01/02 21:01:10, majidvp wrote: > do you actually need to have the test in onload handler? Done. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html:29: logRects(box, true); On 2017/01/02 21:01:10, majidvp wrote: > 'compositor-touch-hit-rects.js' already has facilities for what you are doing > here i.e., adding touchstart handler, forcing compositing, logging the rect). So > I think you could just replace this test with a simple 'preRunHandlerForTest' > That updates the transform. > > See for example: > fast/events/touch/compositor-touch-hit-rects-scroll.html As we discussed, preRunHandlerForTest may not be suitable for this test. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:435: On 2017/01/02 21:01:11, majidvp wrote: > Invalidating touch regions on every transform change for every node feels like > using a BIG hammer. > > Is it possible to limit doing this only for certain transform changes e.g., > perhaps only when the element actually has a 'touchstart' handler? Done. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:437: // should be called to correctly update the touch event flags On 2017/01/02 21:01:11, majidvp wrote: > The comment seems superfluous. It is also a bit incorrect because the logic is > calling notifyGeometryChanged() regardless of whether we need > a layout or not. I think you should just remove the comment. Done. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:438: if (hasTransform) { On 2017/01/02 21:01:10, majidvp wrote: > hmmm, I think this check here is incorrect. You want to recompute > on transform change whether it is adding a transform or removing one. > > In fact, why don't you add a test case where removing the transform causes the > touch target to be recomputed. Done. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:441: scrollingCoordinator->notifyGeometryChanged(); On 2017/01/02 21:01:10, majidvp wrote: > |notifyGeometryChanged| causes recomputation of not only touch regions but also > slow scroll regions. Is this what you wanted? Done. https://codereview.chromium.org/2600593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:443: On 2017/01/02 21:01:10, majidvp wrote: > I rather move this logic at the bottom of the function once transform update > is complete i.e., next to where we notify FrameView of geometry changes. Done.
https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:136: EventTypeNames::touchmove)) After some more thinking I don't think this optimization is correct. In particular, it possible for a paint layer to have touch rects even if does not have touch event handlers as we have an optimization where we report a touch rect on an ancestor non-composited scroller paint layer. In such a case, if a transform changes for this layer, then we should recompute the touch rects. So perhaps instead of looking for existence of touch event handler the right test is to check if this layer (or layout object) has contributed a touch rect. This information is available inside |ScrollingCoordinator::computeTouchEventTargetRects| but it is not kept around. https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:79: void checkTransformChangedDueToTouchEvent(const PaintLayer*); I believe you want the input to be a const reference instead of a pointer. nit: I prefer a name that is consistent with other methods e.g., notifyTransformChanged. The prefixed ToTouchEvent is irrelevant to the method name. https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:424: scrollingCoordinator->checkTransformChangedDueToTouchEvent(this); Is t really enough to notify ScrollingCoordinator when a PaintLayer transform changes? What about cases where a layout object transform is changed but it does not have a layer. I think in those cases we still compute touch rects but in enclosing layer's space. I imagine those rects need to be updated, no?
PTAL. Thanks! https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:136: EventTypeNames::touchmove)) On 2017/01/04 16:48:51, majidvp wrote: > After some more thinking I don't think this optimization is correct. > > In particular, it possible for a paint layer to have touch rects even if does > not have touch event > handlers as we have an optimization where we report a touch rect on an ancestor > non-composited > scroller paint layer. In such a case, if a transform changes for this layer, > then we should > recompute the touch rects. > > > So perhaps instead of looking for existence of touch event handler the right > test is to check if > this layer (or layout object) has contributed a touch rect. This information is > available inside > |ScrollingCoordinator::computeTouchEventTargetRects| but it is not kept around. > As we discussed, we now optimize by walking up the layoutbox tree to see whether the enclosinglayer is in the layerswithrects set. https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:79: void checkTransformChangedDueToTouchEvent(const PaintLayer*); On 2017/01/04 16:48:51, majidvp wrote: > I believe you want the input to be a const reference instead of a pointer. > > nit: I prefer a name that is consistent with other methods e.g., > notifyTransformChanged. The prefixed ToTouchEvent is irrelevant to the method > name. Done. https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2600593002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:424: scrollingCoordinator->checkTransformChangedDueToTouchEvent(this); On 2017/01/04 16:48:51, majidvp wrote: > > Is t really enough to notify ScrollingCoordinator when a PaintLayer transform > changes? > > What about cases where a layout object transform is changed but it does not have > a layer. I think > in those cases we still compute touch rects but in enclosing layer's space. I > imagine those rects > need to be updated, no? > > As we discussed, this part of logic has been moved to layoutBox.
The test file name seems overly verbose to me: compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html Something like compositor-touch-hit-rects-transform-changed-nolayout.html reads better to me. https://codereview.chromium.org/2600593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2600593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:131: void ScrollingCoordinator::notifyTransformChanged(LayoutBox& box) { nit: This should be a const. Our style guides prefers const reference for input parameters (which is this case) or pointers for output params. https://codereview.chromium.org/2600593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:132: if (box.needsLayout()) why is this not |m_page->deprecatedLocalMainFrame()->view()->needsLayout()| similar to other places where we skip this if there is a layout pending. This box may not need layout but if the page roof frame does then we can still skip this. https://codereview.chromium.org/2600593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:141: currentBox = currentBox->parent() && currentBox->parent()->isBox() You are only checking the boxes enclosing layers so it will be more efficient just to walk up the layer tree. Something like: while (Layer* layer = box.enclosingLayer(); layer; layer = layer->parent()) { .... }
Thanks Majid for your feedback:) PTAL. https://codereview.chromium.org/2600593002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2600593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:131: void ScrollingCoordinator::notifyTransformChanged(LayoutBox& box) { On 2017/01/05 16:47:08, majidvp wrote: > nit: This should be a const. Our style guides prefers > const reference for input parameters (which is this case) > or pointers for output params. Done. https://codereview.chromium.org/2600593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:132: if (box.needsLayout()) On 2017/01/05 16:47:08, majidvp wrote: > why is this not |m_page->deprecatedLocalMainFrame()->view()->needsLayout()| > similar to other places where we skip this if there is a layout pending. This > box may not need layout but if the page roof frame does then we can still skip > this. Done. https://codereview.chromium.org/2600593002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:141: currentBox = currentBox->parent() && currentBox->parent()->isBox() On 2017/01/05 16:47:08, majidvp wrote: > You are only checking the boxes enclosing layers so it will be more efficient > just to walk up the layer tree. Something like: > > while (Layer* layer = box.enclosingLayer(); layer; layer = layer->parent()) { > .... > } Done.
lgtm
yigu@chromium.org changed reviewers: + chrishtr@chromium.org
yigu@chromium.org changed required reviewers: + chrishtr@chromium.org
Hi Chris, PTAL at this bug fix. Thanks!
https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:342: scrollingCoordinator->notifyTransformChanged(*this); Just call scrollingCoordinator->notifyGeometryChanged()?
https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:342: scrollingCoordinator->notifyTransformChanged(*this); On 2017/01/09 19:12:46, chrishtr wrote: > Just call scrollingCoordinator->notifyGeometryChanged()? It looks like notifyGeometryChanged() modifies other flags which is not necessary. Plus notifyTransformChanged(.) has other optimization. Right?
On 2017/01/09 at 19:22:02, yigu wrote: > https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:342: scrollingCoordinator->notifyTransformChanged(*this); > On 2017/01/09 19:12:46, chrishtr wrote: > > Just call scrollingCoordinator->notifyGeometryChanged()? > > It looks like notifyGeometryChanged() modifies other flags which is not necessary. Plus notifyTransformChanged(.) has other optimization. Right? In this case, 2D non-composited transforms are being adjusted. That will cause a number of other subsystems to do work, including paint invalidation and raster. I don't think the overhead here will be significant enough to optimize out with a special path, unless you have data showing that.
On 2017/01/09 19:51:29, chrishtr wrote: > On 2017/01/09 at 19:22:02, yigu wrote: > > > https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:342: > scrollingCoordinator->notifyTransformChanged(*this); > > On 2017/01/09 19:12:46, chrishtr wrote: > > > Just call scrollingCoordinator->notifyGeometryChanged()? > > > > It looks like notifyGeometryChanged() modifies other flags which is not > necessary. Plus notifyTransformChanged(.) has other optimization. Right? > > In this case, 2D non-composited transforms are being adjusted. > That will cause a number of other subsystems to do work, including > paint invalidation and raster. I don't think the overhead here will > be significant enough to optimize out with a special path, unless you > have data showing that. We ran tests on a synthetic page which shows that under certain circumstances (transform w/o causing layout or w/o EventListener) is faster with the optimization here. Could you PTAL and see whether the optimization is worth it? In the test, we just move the "translate box". Test and results can be seen at https://docs.google.com/a/google.com/document/d/12ZDzs25S36lv3eyGNxylETDIKeTY...
On 2017/01/11 22:05:53, yigu wrote: > On 2017/01/09 19:51:29, chrishtr wrote: > > On 2017/01/09 at 19:22:02, yigu wrote: > > > > > > https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > > > > > https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:342: > > scrollingCoordinator->notifyTransformChanged(*this); > > > On 2017/01/09 19:12:46, chrishtr wrote: > > > > Just call scrollingCoordinator->notifyGeometryChanged()? > > > > > > It looks like notifyGeometryChanged() modifies other flags which is not > > necessary. Plus notifyTransformChanged(.) has other optimization. Right? > > > > In this case, 2D non-composited transforms are being adjusted. > > That will cause a number of other subsystems to do work, including > > paint invalidation and raster. I don't think the overhead here will > > be significant enough to optimize out with a special path, unless you > > have data showing that. > > We ran tests on a synthetic page which shows that under certain circumstances > (transform w/o causing layout or w/o EventListener) is faster with the > optimization here. Could you PTAL and see whether the optimization is worth it? > In the test, we just move the "translate box". Test and results can be seen at > https://docs.google.com/a/google.com/document/d/12ZDzs25S36lv3eyGNxylETDIKeTY... Thanks for doing the measurement. The test creates 1k divs with touchstart handlers and it shows that cost of computing the rects is about ~20ms in that case (on a rather powerful machine). If I am not mistaken the touch rect computation cost grows linearly with respect to number of handlers so for a page with 50 or so touch handler we are talking ~1ms per frame. The number is obviously larger when considering mobile devices. I think these savings are worth the "small amount" of complexity in this case. Note that without the optimization, any transform change will lead to recomputation of rect even if it was not necessary. This is even more important given that transform changes are considered a cheap way to do all sorts of animations and mutations. So I think going the extra mile to keep them cheap is worth it.
On 2017/01/12 at 20:52:42, majidvp wrote: > On 2017/01/11 22:05:53, yigu wrote: > > On 2017/01/09 19:51:29, chrishtr wrote: > > > On 2017/01/09 at 19:22:02, yigu wrote: > > > > > > > > > https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > > > > > > > > > https://codereview.chromium.org/2600593002/diff/100001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:342: > > > scrollingCoordinator->notifyTransformChanged(*this); > > > > On 2017/01/09 19:12:46, chrishtr wrote: > > > > > Just call scrollingCoordinator->notifyGeometryChanged()? > > > > > > > > It looks like notifyGeometryChanged() modifies other flags which is not > > > necessary. Plus notifyTransformChanged(.) has other optimization. Right? > > > > > > In this case, 2D non-composited transforms are being adjusted. > > > That will cause a number of other subsystems to do work, including > > > paint invalidation and raster. I don't think the overhead here will > > > be significant enough to optimize out with a special path, unless you > > > have data showing that. > > > > We ran tests on a synthetic page which shows that under certain circumstances > > (transform w/o causing layout or w/o EventListener) is faster with the > > optimization here. Could you PTAL and see whether the optimization is worth it? > > In the test, we just move the "translate box". Test and results can be seen at > > https://docs.google.com/a/google.com/document/d/12ZDzs25S36lv3eyGNxylETDIKeTY... > > Thanks for doing the measurement. The test creates 1k divs with touchstart > handlers and it shows that cost of computing the rects is about ~20ms in > that case (on a rather powerful machine). If I am not mistaken the touch rect > computation cost grows linearly with respect to number of handlers so > for a page with 50 or so touch handler we are talking ~1ms per frame. > The number is obviously larger when considering mobile devices. > > I think these savings are worth the "small amount" of complexity in this case. > Note that without the optimization, any transform change will lead to recomputation > of rect even if it was not necessary. > > This is even more important given that transform changes are considered a > cheap way to do all sorts of animations and mutations. So I think going the > extra mile to keep them cheap is worth it. Makes sense. Thanks for the thorough investigation.
lgtm
The CQ bit was checked by yigu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by yigu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, majidvp@chromium.org Link to the patchset: https://codereview.chromium.org/2600593002/#ps120001 (title: "spv2 failure added in FlagExpectations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484320251423680,
"parent_rev": "3b491f68e9be76a22f54208d6c1a3531eb4d8386", "commit_rev":
"3ad9090bc969c9e2b450ec249d4dfac398bb27b8"}
Message was sent while issue was closed.
Description was changed from ========== Fix touch event flag may not be updated correctly by transform which causes no layout BUG=673102 TEST=third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix touch event flag may not be updated correctly by transform which causes no layout BUG=673102 TEST=third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2600593002 Cr-Commit-Position: refs/heads/master@{#443580} Committed: https://chromium.googlesource.com/chromium/src/+/3ad9090bc969c9e2b450ec249d4d... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3ad9090bc969c9e2b450ec249d4d...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2631783003/ by xlai@chromium.org. The reason for reverting is: Suspecting that this CL is causing failure of fast/events/touch/gesture/pad-gesture-fling.html On WebKit Win7 (dbg) because it is the only CL that modifies touch event. https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%....
Message was sent while issue was closed.
Description was changed from ========== Fix touch event flag may not be updated correctly by transform which causes no layout BUG=673102 TEST=third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2600593002 Cr-Commit-Position: refs/heads/master@{#443580} Committed: https://chromium.googlesource.com/chromium/src/+/3ad9090bc969c9e2b450ec249d4d... ========== to ========== Fix touch event flag may not be updated correctly by transform which causes no layout BUG=673102 TEST=third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2600593002 Cr-Commit-Position: refs/heads/master@{#443580} Committed: https://chromium.googlesource.com/chromium/src/+/3ad9090bc969c9e2b450ec249d4d... ==========
On 2017/01/13 20:33:17, xlai (Olivia) wrote: > A revert of this CL (patchset #7 id:120001) has been created in > https://codereview.chromium.org/2631783003/ by mailto:xlai@chromium.org. > > The reason for reverting is: Suspecting that this CL is causing failure of > fast/events/touch/gesture/pad-gesture-fling.html > On WebKit Win7 (dbg) because it is the only CL that > modifies touch event. > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%.... Reland as the failure was not related to this patch.
The CQ bit was checked by yigu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484583455319510,
"parent_rev": "95a1418e25fe0ef90aa7eced41f73e3e7db1f22c", "commit_rev":
"48e70529c038713fe0b2ddd21d555a2791f95071"}
Message was sent while issue was closed.
Description was changed from ========== Fix touch event flag may not be updated correctly by transform which causes no layout BUG=673102 TEST=third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2600593002 Cr-Commit-Position: refs/heads/master@{#443580} Committed: https://chromium.googlesource.com/chromium/src/+/3ad9090bc969c9e2b450ec249d4d... ========== to ========== Fix touch event flag may not be updated correctly by transform which causes no layout BUG=673102 TEST=third_party/WebKit/LayoutTests/fast/events/touch/compositor-touch-hit-rects-geometry-change-as-touch-notified-by-nolayout.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2600593002 Cr-Original-Commit-Position: refs/heads/master@{#443580} Committed: https://chromium.googlesource.com/chromium/src/+/3ad9090bc969c9e2b450ec249d4d... Review-Url: https://codereview.chromium.org/2600593002 Cr-Commit-Position: refs/heads/master@{#443921} Committed: https://chromium.googlesource.com/chromium/src/+/48e70529c038713fe0b2ddd21d55... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/48e70529c038713fe0b2ddd21d55... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
