Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1504923010:
Before finalizing scroll dimensions, check again whether we need scrollbars (Closed)
|
Created:
5 years ago by cbiesinger Modified:
5 years ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBefore finalizing scroll dimensions, check again whether we need scrollbars
Due to other delayed layouts that may have happened in between, we may not need scrollbars anymore.
R=szager@chromium.org,leviw@chromium.org
BUG=560121
Committed: https://crrev.com/657bd17798ed3b25d00771f78a6be52d3fc50507
Cr-Commit-Position: refs/heads/master@{#365741}
Patch Set 1 #Patch Set 2 : tentative better fix #
Total comments: 1
Patch Set 3 : review comment #Patch Set 4 : actually initialize variables at all callers #
Messages
Total messages: 43 (18 generated)
Stefan, I'd like to effectively revert this part of your change. It causes a release blocker - https://code.google.com/p/chromium/issues/detail?id=560121 And, you told me that the original bug is now still not fixed even with this change, so there doesn't seem to be much of a downside to landing this. Let me know your thoughts.
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504923010/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504923010/1
On 2015/12/10 22:47:38, cbiesinger wrote: > Stefan, I'd like to effectively revert this part of your change. It causes a > release blocker - https://code.google.com/p/chromium/issues/detail?id=560121 > > And, you told me that the original bug is now still not fixed even with this > change, so there doesn't seem to be much of a downside to landing this. > > Let me know your thoughts. The original bug would be fixed if this change landed: https://codereview.chromium.org/1470073004 It restores the necessary bit of logic that you removed in: https://codereview.chromium.org/1459743002 You and I are circling each other on this issue. There must be a better fix besides regressing the original bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/12/10 23:52:44, szager1 wrote: > On 2015/12/10 22:47:38, cbiesinger wrote: > > Stefan, I'd like to effectively revert this part of your change. It causes a > > release blocker - https://code.google.com/p/chromium/issues/detail?id=560121 > > > > And, you told me that the original bug is now still not fixed even with this > > change, so there doesn't seem to be much of a downside to landing this. > > > > Let me know your thoughts. > > The original bug would be fixed if this change landed: > > https://codereview.chromium.org/1470073004 > > It restores the necessary bit of logic that you removed in: > > https://codereview.chromium.org/1459743002 > > > You and I are circling each other on this issue. There must be a better fix > besides regressing the original bug. OK, see my comment in that codereview. We can land that, and then it may be sufficient to call updateScrollDimensions again when we're about to call finalizeScrollDimensions, or something along those lines. It does seem that we will have to recompute whether we need to show scrollbars there. Thoughts?
Description was changed from ========== Restore the original scroll update delaying https://codereview.chromium.org/1295933003 did, among other things, split up the scrollbar update delaying so that layout can take into account scrollbars earlier. However, this causes other issues (see the bug for this patch). So this restores the original code in order to fix this issue. It is not clear that updating just scrollbarLogicalWidth(), which is essentially what the change did, without laying out again is correct behavior. R=szager@chromium.org,leviw@chromium.org BUG=560121 ========== to ========== Before finalizing scroll dimensions, check again whether we need scrollbars Due to other delayed layouts that may have happened in between, we may not need scrollbars anymore. R=szager@chromium.org,leviw@chromium.org BUG=560121 ==========
Perhaps something along these lines. It does fix the release blocker, but I need to do a little bit more investigation to convince myself that this is correct.
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504923010/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504923010/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
OK.. this seems to be correct. Stefan/Levi, can you review? Thanks!
https://codereview.chromium.org/1504923010/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1504923010/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:867: bool autoHorizontalScrollBarChanged, autoVerticalScrollBarChanged; I don't think you want new bool's here. Instead: scrollableArea->updateScrollDimensions(scrollInfo.scrollOffset, scrollInfo.autoHorizontalScrollbarChanged, scrollInfo.autoVerticalScrollbarChanged); The logic in updateScrollDimensions should do the right thing with the bool's.
On 2015/12/15 21:43:30, szager1 wrote: > https://codereview.chromium.org/1504923010/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): > > https://codereview.chromium.org/1504923010/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutBlock.cpp:867: bool > autoHorizontalScrollBarChanged, autoVerticalScrollBarChanged; > I don't think you want new bool's here. Instead: > > scrollableArea->updateScrollDimensions(scrollInfo.scrollOffset, > scrollInfo.autoHorizontalScrollbarChanged, > scrollInfo.autoVerticalScrollbarChanged); > > The logic in updateScrollDimensions should do the right thing with the bool's. If I do that, that effectively ignores the return value of the previous updateScrollDimensions because that function just overwrites the value. Consequently, css3/flexbox/flexbox-overflow-auto.html fails with that change.
Stefan, Levi, ping? See above reply.
On 2015/12/16 19:28:22, cbiesinger wrote: > Stefan, Levi, ping? See above reply. I think this should be fixed in updateScrollDimensions by using "|=" instead of "=" to set the bools, i.e.: autoHorizontalScrollbarChanged |= (box().hasAutoHorizontalScrollbar() && ...); autoVerticalScrollbarChanged |= (box().hasAutoVerticalScrollbar() && ...); I'm going to give that a try, maybe you can as well.
On 2015/12/16 19:48:04, szager1 wrote: > On 2015/12/16 19:28:22, cbiesinger wrote: > > Stefan, Levi, ping? See above reply. > > I think this should be fixed in updateScrollDimensions by using "|=" instead of > "=" to set the bools, i.e.: > > autoHorizontalScrollbarChanged |= (box().hasAutoHorizontalScrollbar() && ...); > autoVerticalScrollbarChanged |= (box().hasAutoVerticalScrollbar() && ...); > > I'm going to give that a try, maybe you can as well. OK uploaded a new version with that. Passes css3/flexbox, let's see what the tryservers say. look good now?
yep, lgtm, thanks!
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504923010/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504923010/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Rs-lgtm :)
The CQ bit was checked by leviw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from szager@chromium.org Link to the patchset: https://codereview.chromium.org/1504923010/#ps60001 (title: "actually initialize variables at all callers")
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504923010/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504923010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504923010/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504923010/60001
The CQ bit was unchecked by cbiesinger@google.com
The CQ bit was checked by cbiesinger@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504923010/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504923010/60001
Message was sent while issue was closed.
Description was changed from ========== Before finalizing scroll dimensions, check again whether we need scrollbars Due to other delayed layouts that may have happened in between, we may not need scrollbars anymore. R=szager@chromium.org,leviw@chromium.org BUG=560121 ========== to ========== Before finalizing scroll dimensions, check again whether we need scrollbars Due to other delayed layouts that may have happened in between, we may not need scrollbars anymore. R=szager@chromium.org,leviw@chromium.org BUG=560121 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Before finalizing scroll dimensions, check again whether we need scrollbars Due to other delayed layouts that may have happened in between, we may not need scrollbars anymore. R=szager@chromium.org,leviw@chromium.org BUG=560121 ========== to ========== Before finalizing scroll dimensions, check again whether we need scrollbars Due to other delayed layouts that may have happened in between, we may not need scrollbars anymore. R=szager@chromium.org,leviw@chromium.org BUG=560121 Committed: https://crrev.com/657bd17798ed3b25d00771f78a6be52d3fc50507 Cr-Commit-Position: refs/heads/master@{#365741} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/657bd17798ed3b25d00771f78a6be52d3fc50507 Cr-Commit-Position: refs/heads/master@{#365741} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
