[Missing Tests]: [root layer scrolls] create vertical scrollbar by default |
|||||
Issue descriptionAutomated tests for the below commit have been missing.Would it be possible to add test coverage to avoid regressions in future? CL: ---- https://chromium.googlesource.com/chromium/src.git/+/e3f1cce3d4d5e89a2cc8ff864217fc66e4a4a96a Ref Bug: --------- https://bugs.chromium.org/p/chromium/issues/detail?id=711137 Thank you.
,
Sep 8 2017
,
Sep 11 2017
It might be possible to add a unit test for this that creates a subclass of LayoutBlock which tracks the number of times UpdateBlockLayout is called, and adds an instance of this as a child of the LayoutView.
,
Sep 13 2017
Some observation about UpdateBlockLayout. I wrote a sample html page with lot of text so that scrollbars will be created. Ran this sample test file in content_shell with both RLS on/off. In both cases the LayoutBlock::UpdateBlockLayout isn't called at all. But LayoutBlockFlow::UpdateBlockLayout is called. The count is 10:5 for RLS on/off case. So can we really depend on this count for our unit test?
,
Sep 13 2017
Still i gave a try and made the following unit test case. But there seems to be a problem with AddChild as layouttree modifications are not allowed and it asserts.
LayoutObject.cpp(302)] Check failed: IsAllowedToModifyLayoutTreeStructure(GetDocument())
*******************************************************
#include "core/layout/LayoutBlock.h"
#include "core/layout/LayoutBlockFlow.h"
#include "core/layout/LayoutTestHelper.h"
#include "core/layout/LayoutView.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace blink {
class LayoutCountTest : public RenderingTest {};
class OverrideLayoutBlock : public LayoutBlock {
public:
explicit OverrideLayoutBlock()
: LayoutBlock(NULL) {}
void UpdateBlockLayout(bool relayout_children) override {
layout_count_++;
}
int GetLayoutCount() { return layout_count_; }
private:
int layout_count_;
};
TEST_F(LayoutCountTest, UpdateBlockLayout) {
RuntimeEnabledFeatures::SetRootLayerScrollingEnabled(true);
SetBodyInnerHTML(
"<!DOCTYPE html>"
" <div style='height:1000px'>Item</div>");
OverrideLayoutBlock* layout_block = new OverrideLayoutBlock();
GetLayoutView().AddChild(layout_block);
EXPECT_EQ(1, layout_block->GetLayoutCount());
}
} // namespace blink
************************************************************
,
Sep 13 2017
This is surprising: > The count is 10:5 for RLS on/off case. Can you investigate why there are twice as many calls to LayoutBlockFlow::UpdateBlockLayout with RLS on? I thought they would be the same after r500367.
,
Sep 14 2017
Sorry, i forgot to mention that the above results are without the patch r500367. I reverted it locally to check if LayoutBlock::UpdateBlockLayout is really getting invoked here. The latest results are as expected. It is 3:3 with latest code and including the patch r500367. So should i try the unit test with LayoutBlockFlow instead of LayoutBlock? If so, please have a look at the above unit test code and let me know if the approach is correct. And as i mentioned the layout tree modification (AddChild) isn't allowed. Tried using DeprecatedDisableModifyLayoutTreeStructureAsserts disabler; before addchild() but there is a linking error.
,
Sep 19 2017
Makes sense, thanks for clarifying. It sounds like directly modifying the layout tree isn't a great idea. Instead, how about this: add an entry in runtime_enabled_features.json5 named "TrackLayoutPassesPerBlock". When this feature is enabled, LayoutBlock::UpdateLayout should update a static HashMap<LayoutBlock*, int> which counts the number of times the block has been laid out. Then add a method LayoutBlock::GetLayoutPassCountForTesting that reads from the map. Then the test can enable the REF, run the layout, and verify the count. I think this facility would be useful in other places as well. For example, the flexbox implementation relies on multiple layout passes. The concept is similar to GraphicsLayer's RasterInvalidationTrackingMap which is used by all of the repaint-tracking layout tests (internals.startTrackingRepaints).
,
Sep 19 2017
Thanks, i will try that.
,
Sep 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b309ad450b0f4b62f204643f7b5b35bfc0f52ec commit 8b309ad450b0f4b62f204643f7b5b35bfc0f52ec Author: Sriram <srirama.m@samsung.com> Date: Thu Sep 28 04:38:24 2017 [RLS] Add test to track layout pass count Add test coverage to make sure there isn't an extra layout for a page which requires scrollbar. Scrollbar is created by default to avoid extra layout incase of RLS. Bug: 763341 Change-Id: I3e935dee974429fedf1c7ef41e9d3ec19cfb1698 Reviewed-on: https://chromium-review.googlesource.com/680194 Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Steve Kobes <skobes@chromium.org> Commit-Queue: srirama chandra sekhar <srirama.m@samsung.com> Cr-Commit-Position: refs/heads/master@{#504900} [modify] https://crrev.com/8b309ad450b0f4b62f204643f7b5b35bfc0f52ec/third_party/WebKit/Source/core/BUILD.gn [modify] https://crrev.com/8b309ad450b0f4b62f204643f7b5b35bfc0f52ec/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp [modify] https://crrev.com/8b309ad450b0f4b62f204643f7b5b35bfc0f52ec/third_party/WebKit/Source/core/layout/LayoutBlockFlow.h [add] https://crrev.com/8b309ad450b0f4b62f204643f7b5b35bfc0f52ec/third_party/WebKit/Source/core/layout/LayoutCountTest.cpp [modify] https://crrev.com/8b309ad450b0f4b62f204643f7b5b35bfc0f52ec/third_party/WebKit/Source/platform/runtime_enabled_features.json5
,
Sep 28 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by sriram...@samsung.com
, Sep 8 2017