Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2575343004:
Invalidate LayoutView when the URL bar is hidden on short pages. (Closed)
DescriptionInvalidate LayoutView when the URL bar is hidden on short pages.
On a pages with no scrolling, hiding the top controls exposes a region beyond
the document rect. We need to invalidate this region so that we paint the
background into it.
This isn't a problem on scrollable pages because we simply reveal more of the
content layer. Short pages are revealing a region of nothingness.
In addition, I caught a mistake I made in a previous patch, in
https://codereview.chromium.org/2461463004. The width|heightChanged arguments
passed into setShouldDoFullPaintInvalidationOnResizeIfNeeded in
LayoutView::layout should compare against the layoutSize rather than the
visibleContentSize so I fixed the mistake.
BUG=670853
Committed: https://crrev.com/f9f732d6fc99d6ddbff14333f17c0b5978e22322
Cr-Commit-Position: refs/heads/master@{#440527}
Patch Set 1 #Patch Set 2 : NA #Patch Set 3 : Invalidate only for main frame and if URL bar exists #Patch Set 4 : Fixed mistake from an older patch #Patch Set 5 : Nit #
Total comments: 2
Messages
Total messages: 30 (20 generated)
bokan@chromium.org changed reviewers: + skobes@chromium.org
lgtm
The CQ bit was checked by bokan@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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bokan@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by bokan@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 ========== Invalidate LayoutView when the URL bar is hidden on short pages. On a pages with no scrolling, hiding the top controls exposes a region beyond the document rect. We need to invalidate this region so that we paint the background into it. This isn't a problem on scrollable pages because we simply reveal more of the content layer. Short pages are revealing a region of nothingness. BUG=670853 ========== to ========== Invalidate LayoutView when the URL bar is hidden on short pages. On a pages with no scrolling, hiding the top controls exposes a region beyond the document rect. We need to invalidate this region so that we paint the background into it. This isn't a problem on scrollable pages because we simply reveal more of the content layer. Short pages are revealing a region of nothingness. In addition, I caught a mistake I made in a previous patch, in https://codereview.chromium.org/2461463004. The width|heightChanged arguments passed into setShouldDoFullPaintInvalidationOnResizeIfNeeded in LayoutView::layout should compare against the layoutSize rather than the visibleContentSize so I fixed the mistake. BUG=670853 ==========
Description was changed from ========== Invalidate LayoutView when the URL bar is hidden on short pages. On a pages with no scrolling, hiding the top controls exposes a region beyond the document rect. We need to invalidate this region so that we paint the background into it. This isn't a problem on scrollable pages because we simply reveal more of the content layer. Short pages are revealing a region of nothingness. In addition, I caught a mistake I made in a previous patch, in https://codereview.chromium.org/2461463004. The width|heightChanged arguments passed into setShouldDoFullPaintInvalidationOnResizeIfNeeded in LayoutView::layout should compare against the layoutSize rather than the visibleContentSize so I fixed the mistake. BUG=670853 ========== to ========== Invalidate LayoutView when the URL bar is hidden on short pages. On a pages with no scrolling, hiding the top controls exposes a region beyond the document rect. We need to invalidate this region so that we paint the background into it. This isn't a problem on scrollable pages because we simply reveal more of the content layer. Short pages are revealing a region of nothingness. In addition, I caught a mistake I made in a previous patch, in https://codereview.chromium.org/2461463004. The width|heightChanged arguments passed into setShouldDoFullPaintInvalidationOnResizeIfNeeded in LayoutView::layout should compare against the layoutSize rather than the visibleContentSize so I fixed the mistake. BUG=670853 ==========
The CQ bit was checked by bokan@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.
Steve, ptal? I made the condition for invalidation more strict so that we don't overinvalidate. In the process, I discovered a mistake in an old patch so I fixed that as well. Still ok? (details in comments -- I'll rename viewWidth/Height in a followup to prevent the same mistake in the future) https://codereview.chromium.org/2575343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2575343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:1590: m_frame->isMainFrame() && m_frame->host()->browserControls().height()) { This doesn't really apply to non-main frames (yet, document.rootScroller will likely change that) and even for main frames should only occur when we're using a mobile URL bar. I've added these conditions to prevent over-invalidation since without the URL bar we'll invalidate from layout. I don't think it's an issue but this prevents having to rebase a bunch of layout tests that don't expect multiple invalidations. https://codereview.chromium.org/2575343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutView.cpp (right): https://codereview.chromium.org/2575343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutView.cpp:233: offsetWidth() != layoutSize(includeScrollbars).width(), In https://codereview.chromium.org/2461463004, I moved these `widthChanged`|`heightChanged` calculations from setShouldDoFullPaintInvalidationOnResizeIfNeeded to the call sites and passed them as parameters. I made a mistake though in that I assumed viewWidth|viewHeight == visibleContentSize -- they are not. viewWidth|viewHeight are actually layoutWidth and layoutHeight. The mistake wasn't caught because it was masked by the invalidation that happens in FrameView::viewportSizeChanged. Now that I've tightened that up to happen only when there's a mobile URL bar it caused window-resize-background-image-generated|fixed.html to fail.
Just noticed you're OOO for the next few days. I'm going to land now, the changes are fairly minor and (I expect) non controversial. Happy to make changes after the fact if you like.
The CQ bit was checked by bokan@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/2575343004/#ps80001 (title: "Nit")
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": 80001, "attempt_start_ts": 1482445691805500,
"parent_rev": "43c96268017edda37211397426220abe79c33559", "commit_rev":
"ec905a0712f0674723b79c83f207f4debecad550"}
Message was sent while issue was closed.
Description was changed from ========== Invalidate LayoutView when the URL bar is hidden on short pages. On a pages with no scrolling, hiding the top controls exposes a region beyond the document rect. We need to invalidate this region so that we paint the background into it. This isn't a problem on scrollable pages because we simply reveal more of the content layer. Short pages are revealing a region of nothingness. In addition, I caught a mistake I made in a previous patch, in https://codereview.chromium.org/2461463004. The width|heightChanged arguments passed into setShouldDoFullPaintInvalidationOnResizeIfNeeded in LayoutView::layout should compare against the layoutSize rather than the visibleContentSize so I fixed the mistake. BUG=670853 ========== to ========== Invalidate LayoutView when the URL bar is hidden on short pages. On a pages with no scrolling, hiding the top controls exposes a region beyond the document rect. We need to invalidate this region so that we paint the background into it. This isn't a problem on scrollable pages because we simply reveal more of the content layer. Short pages are revealing a region of nothingness. In addition, I caught a mistake I made in a previous patch, in https://codereview.chromium.org/2461463004. The width|heightChanged arguments passed into setShouldDoFullPaintInvalidationOnResizeIfNeeded in LayoutView::layout should compare against the layoutSize rather than the visibleContentSize so I fixed the mistake. BUG=670853 Review-Url: https://codereview.chromium.org/2575343004 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Invalidate LayoutView when the URL bar is hidden on short pages. On a pages with no scrolling, hiding the top controls exposes a region beyond the document rect. We need to invalidate this region so that we paint the background into it. This isn't a problem on scrollable pages because we simply reveal more of the content layer. Short pages are revealing a region of nothingness. In addition, I caught a mistake I made in a previous patch, in https://codereview.chromium.org/2461463004. The width|heightChanged arguments passed into setShouldDoFullPaintInvalidationOnResizeIfNeeded in LayoutView::layout should compare against the layoutSize rather than the visibleContentSize so I fixed the mistake. BUG=670853 Review-Url: https://codereview.chromium.org/2575343004 ========== to ========== Invalidate LayoutView when the URL bar is hidden on short pages. On a pages with no scrolling, hiding the top controls exposes a region beyond the document rect. We need to invalidate this region so that we paint the background into it. This isn't a problem on scrollable pages because we simply reveal more of the content layer. Short pages are revealing a region of nothingness. In addition, I caught a mistake I made in a previous patch, in https://codereview.chromium.org/2461463004. The width|heightChanged arguments passed into setShouldDoFullPaintInvalidationOnResizeIfNeeded in LayoutView::layout should compare against the layoutSize rather than the visibleContentSize so I fixed the mistake. BUG=670853 Committed: https://crrev.com/f9f732d6fc99d6ddbff14333f17c0b5978e22322 Cr-Commit-Position: refs/heads/master@{#440527} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f9f732d6fc99d6ddbff14333f17c0b5978e22322 Cr-Commit-Position: refs/heads/master@{#440527}
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
