|
|
Description[RootLayerScrolls] Add convenience methods for PLSA visible client size.
This anticipates when the size of the main frame's root layer is based on
the main frame's layout size.
BUG=701575
R=skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2841113002
Cr-Commit-Position: refs/heads/master@{#468062}
Committed: https://chromium.googlesource.com/chromium/src/+/9309126a1a26b15343a2db4e2a5358dcaa9bd9f8
Patch Set 1 #Patch Set 2 : Use FrameView's layout size for visible client size. #Patch Set 3 : Test rebaselines #Patch Set 4 : Revert inadvertent snapping #Patch Set 5 : cleanup #
Total comments: 2
Patch Set 6 : Exclude scrollbars from client size #Patch Set 7 : comment #
Messages
Total messages: 32 (21 generated)
Description was changed from ========== Add convenience methods for PLSA visible client dimensions. BUG=701575 R=skobes@chromium.org ========== to ========== Add convenience methods for PLSA visible client dimensions. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by szager@chromium.org to run a CQ dry run
Dry run: 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
Dry run: 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_...) mac_chromium_rel_ng on master.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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add convenience methods for PLSA visible client dimensions. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RootLayerScrolls] Add convenience methods for PLSA visible client size. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== [RootLayerScrolls] Add convenience methods for PLSA visible client size. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RootLayerScrolls] Add convenience methods for PLSA visible client size. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by szager@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [RootLayerScrolls] Add convenience methods for PLSA visible client size. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RootLayerScrolls] Add convenience methods for PLSA visible client size. This anticipates when the size of the main frame's root layer is based on the main frame's layout size. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== [RootLayerScrolls] Add convenience methods for PLSA visible client size. This anticipates when the size of the main frame's root layer is based on the main frame's layout size. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RootLayerScrolls] Add convenience methods for PLSA visible client size. This anticipates when the size of the main frame's root layer is based on the main frame's layout size. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Why is custom-scrollbar-display-expected.png added here? Is it different from the non-virtual baseline? https://codereview.chromium.org/2841113002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2841113002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:300: LayoutSize ClientSize() const; Isn't this the same as VisibleContentRect().Size()? Should we call it VisibleContentSize for consistency?
On 2017/04/27 23:22:47, skobes wrote: > Why is custom-scrollbar-display-expected.png added here? Is it different from > the non-virtual baseline? With this change, the virtual test paints the scrollbars slightly larger than the non-virtual test. I would prefer to kick this can down the road and solve it along with all of the other RLS layout test failures, after the layer size stuff is landed.
https://codereview.chromium.org/2841113002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2841113002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:300: LayoutSize ClientSize() const; On 2017/04/27 23:22:47, skobes wrote: > Isn't this the same as VisibleContentRect().Size()? Should we call it > VisibleContentSize for consistency? VisibleContentRect is based on layer_.Size(), which gets updated at a different time from Box().ClientWidth(). The methods that use ClientSize() may run after box layout but before UpdateLayerPositionsAfterLayout(). At the end of this series of patches, the box and layer will get their sizes simultaneously (from the point of view of PLSA). When that happens, the code can (and should) use layer size everywhere. The name ClientSize is temporary; I chose it because it mostly replaces calls to Box().[PixelSnapped]Client(Width|Height), and communicates the fact that it's based on box size, so is safe to call from UpdateAfterLayout. I don't think it's worth bikeshedding over the name, so I'll do whatever you prefer.
The CQ bit was checked by szager@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/28 11:01:28, szager1 wrote: > On 2017/04/27 23:22:47, skobes wrote: > > Why is custom-scrollbar-display-expected.png added here? Is it different from > > the non-virtual baseline? > > With this change, the virtual test paints the scrollbars slightly larger than > the non-virtual test. I would prefer to kick this can down the road and solve > it along with all of the other RLS layout test failures, after the layer size > stuff is landed. I dug into this and found the bug: with RLS enabled, we get the client size from the FrameView, but must also subtract the scrollbar width/height from the root PLSA. Test now passes, I removed the updated expectation.
On 2017/04/28 11:04:53, szager1 wrote: > On 2017/04/27 23:22:47, skobes wrote: > > Isn't this the same as VisibleContentRect().Size()? Should we call it > > VisibleContentSize for consistency? > > VisibleContentRect is based on layer_.Size(), which gets updated at a different > time from Box().ClientWidth(). The methods that use ClientSize() may run after > box layout but before UpdateLayerPositionsAfterLayout(). > > At the end of this series of patches, the box and layer will get their sizes > simultaneously (from the point of view of PLSA). When that happens, the code > can (and should) use layer size everywhere. This sounds great :) > The name ClientSize is temporary; I chose it because it mostly replaces calls to > Box().[PixelSnapped]Client(Width|Height), and communicates the fact that it's > based on box size, so is safe to call from UpdateAfterLayout. Do you mean the method is temporary and will be removed in the end? Or that it will get a different name? If the method will stick around we should pick a name that makes sense for the end state; I like VisibleContentSize to parallel VisibleContentRect. If the method will be removed I don't care what it's called but please add a comment saying it's temporary.
On 2017/04/28 15:25:11, skobes wrote: > On 2017/04/28 11:04:53, szager1 wrote: > > On 2017/04/27 23:22:47, skobes wrote: > > > Isn't this the same as VisibleContentRect().Size()? Should we call it > > > VisibleContentSize for consistency? > > > > VisibleContentRect is based on layer_.Size(), which gets updated at a > different > > time from Box().ClientWidth(). The methods that use ClientSize() may run > after > > box layout but before UpdateLayerPositionsAfterLayout(). > > > > At the end of this series of patches, the box and layer will get their sizes > > simultaneously (from the point of view of PLSA). When that happens, the code > > can (and should) use layer size everywhere. > > This sounds great :) > > > The name ClientSize is temporary; I chose it because it mostly replaces calls > to > > Box().[PixelSnapped]Client(Width|Height), and communicates the fact that it's > > based on box size, so is safe to call from UpdateAfterLayout. > > Do you mean the method is temporary and will be removed in the end? Or that it > will get a different name? > > If the method will stick around we should pick a name that makes sense for the > end state; I like VisibleContentSize to parallel VisibleContentRect. If the > method will be removed I don't care what it's called but please add a comment > saying it's temporary. I mean that at the end, we can get rid of it and just just VisibleContentRect(). If we want to add a convenience method VisibleContentSize() { return VisibleContentRect().Size() }, we can do that later. But this method is strictly temporary.
lgtm w/ comment saying it's temporary until VisibleContentRect is fixed
On 2017/04/28 15:56:37, skobes wrote: > lgtm w/ comment saying it's temporary until VisibleContentRect is fixed Done.
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org Link to the patchset: https://codereview.chromium.org/2841113002/#ps120001 (title: "comment")
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": 1493395610098580, "parent_rev": "1d19b802b839f71f5d077fe3edb92e7902f4db52", "commit_rev": "9309126a1a26b15343a2db4e2a5358dcaa9bd9f8"}
Message was sent while issue was closed.
Description was changed from ========== [RootLayerScrolls] Add convenience methods for PLSA visible client size. This anticipates when the size of the main frame's root layer is based on the main frame's layout size. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RootLayerScrolls] Add convenience methods for PLSA visible client size. This anticipates when the size of the main frame's root layer is based on the main frame's layout size. BUG=701575 R=skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2841113002 Cr-Commit-Position: refs/heads/master@{#468062} Committed: https://chromium.googlesource.com/chromium/src/+/9309126a1a26b15343a2db4e2a53... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/9309126a1a26b15343a2db4e2a53... |