Bug 105486

Summary: Need to re-layout fixed position elements after scale when using settings()->fixedElementsLayoutRelativeToFrame()
Product: WebKit Reporter: Tien-Ren Chen <trchen>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aelias, buildbot, dglazkov, eric, jamesr, jchaffraix, ojan.autocc, rniwa, simon.fraser, skyostil, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 110491    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Tien-Ren Chen 2012-12-19 17:33:09 PST
Need to re-layout fixed position elements after scale when using settings()->fixedElementsLayoutRelativeToFrame()
Comment 1 Tien-Ren Chen 2012-12-19 17:42:29 PST
Created attachment 180254 [details]
Patch
Comment 2 Sami Kyöstilä 2012-12-20 03:02:52 PST
Comment on attachment 180254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180254&action=review

> Source/WebCore/rendering/RenderView.cpp:1042
> +        RenderBox* r = *it;

Nit: use full word variables.

> Source/WebCore/rendering/RenderView.h:231
> +    void markFixedPositionDescendantsNeedsMovementLayout();

Bikeshed: markFixedPositionDescendantsNeedMovementLayout sounds less awkward to me.

> LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:32
> +        window.internals.settings.setFixedElementsLayoutRelativeToFrame(true)

Nit: missing semicolon.
Comment 3 Sami Kyöstilä 2012-12-20 03:06:07 PST
Oops, hit submit too soon. I think this fixes the problem nicely -- thanks Tien-Ren.
Comment 4 Tien-Ren Chen 2012-12-20 14:00:41 PST
Created attachment 180405 [details]
Patch
Comment 5 Tien-Ren Chen 2012-12-20 14:01:32 PST
(In reply to comment #4)
> Created an attachment (id=180405) [details]
> Patch

Fixed the nits Sami mentioned. Thanks!
Comment 6 Simon Fraser (smfr) 2012-12-20 14:32:01 PST
Comment on attachment 180405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180405&action=review

> Source/WebCore/rendering/RenderView.cpp:1045
> +    TrackedRendererListHashSet* positionedDescendants = positionedObjects();
> +    if (!positionedDescendants)
> +        return;
> +    TrackedRendererListHashSet::iterator end = positionedDescendants->end();
> +    for (TrackedRendererListHashSet::iterator it = positionedDescendants->begin(); it != end; ++it) {
> +        RenderBox* renderer = *it;
> +        if (renderer->style()->position() != FixedPosition)
> +            continue;
> +        renderer->setNeedsPositionedMovementLayout();

You should use FrameView's list of fixed objects, not this one.
Comment 7 Tien-Ren Chen 2012-12-20 14:37:34 PST
(In reply to comment #6)
> (From update of attachment 180405 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180405&action=review
> 
> > Source/WebCore/rendering/RenderView.cpp:1045
> > +    TrackedRendererListHashSet* positionedDescendants = positionedObjects();
> > +    if (!positionedDescendants)
> > +        return;
> > +    TrackedRendererListHashSet::iterator end = positionedDescendants->end();
> > +    for (TrackedRendererListHashSet::iterator it = positionedDescendants->begin(); it != end; ++it) {
> > +        RenderBox* renderer = *it;
> > +        if (renderer->style()->position() != FixedPosition)
> > +            continue;
> > +        renderer->setNeedsPositionedMovementLayout();
> 
> You should use FrameView's list of fixed objects, not this one.

Ah, that's a really good suggestion! I'll change it.

By the way, I'm thinking to remove the USE(ACCELERATED_COMPOSITING) guard on Frame::deviceOrPageScaleFactorChanged() so the re-layout will be triggered from there instead of Page. Do you think that sounds like a better plan?
Comment 8 Tien-Ren Chen 2012-12-20 16:39:41 PST
Created attachment 180440 [details]
Patch
Comment 9 Simon Fraser (smfr) 2012-12-20 17:39:23 PST
Comment on attachment 180440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180440&action=review

> LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:1
> +<!DOCTYPE html>

The image result has non-mock scrollbars, which will not match most platforms.

Can this be a ref test?
Comment 10 Tien-Ren Chen 2012-12-20 18:26:24 PST
Created attachment 180454 [details]
Patch
Comment 11 Simon Fraser (smfr) 2012-12-20 19:21:35 PST
Comment on attachment 180454 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180454&action=review

> Source/WebCore/ChangeLog:17
> +        (WebCore):

Remove useless lines like this.

> LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:9
> +::-webkit-scrollbar {
> +    width:0;
> +    height:0;
> +}

This doesn't work in Mac WK1. body { overflow: hidden; } is a better way to hide scrollbars.
Comment 12 Tien-Ren Chen 2012-12-20 19:31:52 PST
(In reply to comment #11)
> (From update of attachment 180454 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180454&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        (WebCore):
> 
> Remove useless lines like this.

Roger that.

> > LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:9
> > +::-webkit-scrollbar {
> > +    width:0;
> > +    height:0;
> > +}
> 
> This doesn't work in Mac WK1. body { overflow: hidden; } is a better way to hide scrollbars.

This doesn't work with scaled page. I guess the reason is that overflow:hidden only shrink <body> to the layout viewport size.

Let's just add a Skip in TestExpectations?
Comment 13 Tien-Ren Chen 2012-12-20 19:38:03 PST
Created attachment 180462 [details]
Patch
Comment 14 Simon Fraser (smfr) 2013-01-02 21:01:00 PST
(In reply to comment #12)
> Let's just add a Skip in TestExpectations?

Better to just list it as an image failure.
Comment 15 Tien-Ren Chen 2013-01-28 19:21:39 PST
Created attachment 185133 [details]
Patch
Comment 16 Tien-Ren Chen 2013-01-28 19:25:07 PST
Changes from last patch:
* Changed the hook point from Frame::deviceOrPageScaleFactorChanged() to FrameView::visibleContentsResized(). This makes more sense and make code much cleaner.
* In the layout test, added a trick to force re-layout before changing scale factor again so we can avoid scrollbars.
* Rebased.
Comment 17 Sami Kyöstilä 2013-01-29 05:35:30 PST
Comment on attachment 185133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185133&action=review

I think the fix itself looks good. Just one tiny comment about the test.

> LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:36
> +        window.internals.boundingBox(document.getElementsByTagName("body")[0]);

Nit: the other tests I've seen use something like element.offsetTop to force a layout. I think that would be a little less obscure.
Comment 18 Build Bot 2013-01-29 09:21:34 PST
Comment on attachment 185133 [details]
Patch

Attachment 185133 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16200216

New failing tests:
fast/repaint/relayout-fixed-position-after-scale.html
Comment 19 Tien-Ren Chen 2013-01-29 16:03:09 PST
Created attachment 185330 [details]
Patch
Comment 20 Tien-Ren Chen 2013-01-29 16:06:14 PST
Let's make the test chromium-specific. The mac build doesn't seem to update visibleContentRect when setPageScaleFactor is called. They use a significant different approach anyway.
Comment 21 Tien-Ren Chen 2013-01-30 17:38:29 PST
Created attachment 185636 [details]
Patch
Comment 22 Tien-Ren Chen 2013-01-30 17:39:31 PST
Created attachment 185637 [details]
Patch
Comment 23 Tien-Ren Chen 2013-01-30 17:40:33 PST
Comment on attachment 185637 [details]
Patch

Use document.body.offsetTop magic spell as suggested by Sami. I think this patch is ready. PTAL, thanks!
Comment 24 Tien-Ren Chen 2013-02-01 13:40:52 PST
Hello Simon,
Do you have time to take a look? I hope the patch doesn't look too sketchy to you. :)
Comment 25 Tien-Ren Chen 2013-02-04 18:48:21 PST
Created attachment 186521 [details]
Patch
Comment 26 Tien-Ren Chen 2013-02-04 18:53:14 PST
Created attachment 186522 [details]
Patch
Comment 27 Tien-Ren Chen 2013-02-04 18:53:40 PST
Comment on attachment 186522 [details]
Patch

Last one was a bad rebase. This one is correct.
Comment 28 Alexandre Elias 2013-02-04 18:55:39 PST
Looks good to me, thanks.
Comment 29 Tony Chang 2013-02-05 10:10:39 PST
Comment on attachment 186522 [details]
Patch

Why is the new test in platform/chromium when the code change is in WebCore?  Is there a reason that this test won't pass on other platforms?
Comment 30 Tien-Ren Chen 2013-02-05 11:33:50 PST
(In reply to comment #29)
> (From update of attachment 186522 [details])
> Why is the new test in platform/chromium when the code change is in WebCore?  Is there a reason that this test won't pass on other platforms?

One reason is because there is no reliable way to completely hide scrollbar on Mac. Setting <body style="overflow:hidden"> won't work when a page is scaled, and the Mac doesn't support -webkit-scrollbar. Being unable to hide scrollbar, the visible content size becomes an odd number (785, 585), and rounding errors come up when page is scaled, making it impossible to write a ref-test that works.

Another reason is that Mac seems to treat visibleContentRect differently. When I ran the test on a Macbook, FrameView::visibleContentsResized() wasn't called when setPageScaleFactor is called.

I don't usually make tests platform-specific, but the Mac port simply works too differently in this case.
Comment 31 Tony Chang 2013-02-05 12:29:00 PST
(In reply to comment #30)
> One reason is because there is no reliable way to completely hide scrollbar on Mac. Setting <body style="overflow:hidden"> won't work when a page is scaled, and the Mac doesn't support -webkit-scrollbar. Being unable to hide scrollbar, the visible content size becomes an odd number (785, 585), and rounding errors come up when page is scaled, making it impossible to write a ref-test that works.

You could try to use overflow:overlay, which should draw scrollbars that don't take up space.


> Another reason is that Mac seems to treat visibleContentRect differently. When I ran the test on a Macbook, FrameView::visibleContentsResized() wasn't called when setPageScaleFactor is called.

I don't know enough about this code or whether this is a bug or not.

> I don't usually make tests platform-specific, but the Mac port simply works too differently in this case.

Please mention the reasons for a platform specific test in the ChangeLog.
Comment 32 Simon Fraser (smfr) 2013-02-05 12:57:54 PST
(In reply to comment #31)
> (In reply to comment #30)
> > One reason is because there is no reliable way to completely hide scrollbar on Mac. Setting <body style="overflow:hidden"> won't work when a page is scaled, and the Mac doesn't support -webkit-scrollbar. Being unable to hide scrollbar, the visible content size becomes an odd number (785, 585), and rounding errors come up when page is scaled, making it impossible to write a ref-test that works.
> 
> You could try to use overflow:overlay, which should draw scrollbars that don't take up space.

Please don't, we're going to obsolete that value at some point.

Custom scrollbars work in WK2, so use them if you have to but you'll have to mark WK1 tests as expected failures.
Comment 33 Tien-Ren Chen 2013-02-05 13:13:01 PST
Created attachment 186688 [details]
Patch
Comment 34 Tien-Ren Chen 2013-02-05 13:13:52 PST
Comment on attachment 186688 [details]
Patch

Revised ChangeLog as suggested.
Comment 35 Tien-Ren Chen 2013-02-13 20:23:49 PST
Ping. Can I haz a r+ please? :3
Comment 36 Alexandre Elias 2013-02-13 20:25:29 PST
Could you try moving the call into setPageScaleFactor()?  This seems like a more indirect way of achieving the same thing.
Comment 37 Tien-Ren Chen 2013-02-13 22:00:57 PST
(In reply to comment #36)
> Could you try moving the call into setPageScaleFactor()?  This seems like a more indirect way of achieving the same thing.

I think visibleContentsResized is the right place, since settings()->fixedElementsLayoutRelativeToFrame() == true means fixed-position elements should stick to the visible content rect. This is the perfect single place to add this check rather than adding it everywhere that can potentially change the visible content size.

The bug title may not accurately describe what this patch does, but it reflects a user-visible bug, and the test reproduces the bug well.
Comment 38 James Robinson 2013-02-14 10:42:57 PST
Comment on attachment 186688 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186688&action=review

> Source/WebCore/page/FrameView.cpp:2026
> +    if (fixedElementsLayoutRelativeToFrame())
> +        setViewportConstrainedObjectsNeedLayout();

I don't think this is the right fix for the problem you are seeing.  visibleContentsResized() is called from ScrollView::updateScrollbars() so that the view can react to the changes in visible content size due to overflow controls being created/destroyed.  In your case, you have overlay scrollbars so the overflow controls do not change the amount of content space or visible content space.  Thus there's no reason to react to visibleContentsResized since they haven't actually resized.  I suspect you observe that this patch papers over the bug you have since it's called frequently and happens to trigger in the case where you actually do need additional layout.

You need to figure out what is actually changing and why that is not triggering the layout you need.
Comment 39 Tien-Ren Chen 2013-02-14 11:16:14 PST
(In reply to comment #38)
> (From update of attachment 186688 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186688&action=review
> 
> > Source/WebCore/page/FrameView.cpp:2026
> > +    if (fixedElementsLayoutRelativeToFrame())
> > +        setViewportConstrainedObjectsNeedLayout();
> 
> I don't think this is the right fix for the problem you are seeing.  visibleContentsResized() is called from ScrollView::updateScrollbars() so that the view can react to the changes in visible content size due to overflow controls being created/destroyed.  In your case, you have overlay scrollbars so the overflow controls do not change the amount of content space or visible content space.  Thus there's no reason to react to visibleContentsResized since they haven't actually resized.  I suspect you observe that this patch papers over the bug you have since it's called frequently and happens to trigger in the case where you actually do need additional layout.
> 
> You need to figure out what is actually changing and why that is not triggering the layout you need.

I consider that as a separate issue. If visibleContentsResized() is not called when it should be called, we should fix that as well, or we should mark it as obsolete if it's broken beyond fix.

I agree that currently how the event propagates to visibleContentsResized is under the brittle assumption that changing the frame rect always need to recompute the scrollbar and just do it on the way. Actually I think call it in ScrollView::updateScrollbars() isn't such a wrong way to do it, as non-overlay scrollbars do change visibleContentsRect(false).

It is perfectly correct to react to visibleContentsResized. If they haven't actually resized, then we don't need to re-layout fixed position elements either. My point is that if visible content size is not resized when it should, we should fix that in a separate patch.

If you ask my opinion about how to do visible contents size correctly, I would say we should make it a stored value instead of a computed value. I personally hate computed value when you need to get the change notifications. If we make it a stored value, we can emit the notifications in setVisibleContentsSize(). 

Or another way of thinking is that we should avoid to source fixed-position layout from multiple value, but using just one authoritative FrameView::viewportConstrainedVisibleContentRect (make it a stored value). That was what I proposed in the comments of webkit.org/b/108323 .

Either way, it won't be in the scope of this patch. fixedElementsLayoutRelativeToFrame()==true means use the visibleContentsSize to layout fixed position elements, so we re-layout fixed-position elements when it changes. Simple. I strongly suggest this code should be in, unless we're going to change the semantics of fixedElementsLayoutRelativeToFrame() in near future.
Comment 40 Tien-Ren Chen 2013-02-19 18:05:35 PST
Created attachment 189217 [details]
Patch
Comment 41 Tien-Ren Chen 2013-02-19 18:14:48 PST
Changes from last patch:
* Added FrameView::visibleContentScaleFactorDidChange() notification to make sure FrameView::visibleContentsResized() is called during Page::setPageScaleFactor.
* Added guard against unnecessary re-layout

(In reply to comment #38)
> (From update of attachment 186688 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186688&action=review
> 
> > Source/WebCore/page/FrameView.cpp:2026
> > +    if (fixedElementsLayoutRelativeToFrame())
> > +        setViewportConstrainedObjectsNeedLayout();
> 
> I don't think this is the right fix for the problem you are seeing.  visibleContentsResized() is called from ScrollView::updateScrollbars() so that the view can react to the changes in visible content size due to overflow controls being created/destroyed.  In your case, you have overlay scrollbars so the overflow controls do not change the amount of content space or visible content space.  Thus there's no reason to react to visibleContentsResized since they haven't actually resized.  I suspect you observe that this patch papers over the bug you have since it's called frequently and happens to trigger in the case where you actually do need additional layout.
> 
> You need to figure out what is actually changing and why that is not triggering the layout you need.

FrameView::visibleContentsResized() is currently called from this path as a side effect:
Page::setPageScaleFactor
  FrameView::setScrollPosition
    ScrollView::setScrollPosition
      ScrollView::updateScrollbars
        FrameView::visibleContentsResized

I agree although it works in current shape it is not necessarily correct. Adding another notification for visible content scale change would make the code more robust.
Comment 42 James Robinson 2013-02-19 18:14:51 PST
Comment on attachment 189217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review

> Source/WebCore/page/Page.cpp:751
> +    view->visibleContentScaleFactorDidChange();

this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same?
Comment 43 Tien-Ren Chen 2013-02-19 18:17:05 PST
(In reply to comment #42)
> (From update of attachment 189217 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review
> 
> > Source/WebCore/page/Page.cpp:751
> > +    view->visibleContentScaleFactorDidChange();
> 
> this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same?

FrameView::visibleContentsResized() gets called a lot. Scrolling without changing scrollbar visibility? Still get called. Changing overlay scrollbar visibility? Still get called.

I would say we'd better apply the same guard on the whole function, but I don't know what are the possible side effects.
Comment 44 WebKit Review Bot 2013-02-19 19:13:06 PST
Comment on attachment 189217 [details]
Patch

Attachment 189217 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16640112

New failing tests:
platform/chromium/fast/repaint/relayout-fixed-position-after-scale.html
Comment 45 Tien-Ren Chen 2013-02-19 19:59:59 PST
Created attachment 189224 [details]
Patch
Comment 46 Tien-Ren Chen 2013-02-19 20:03:38 PST
Changes from last patch:
* Corrects old pinch mode (applyPageScaleFactorInCompositor() == false) compatibility for the viewport-constrained visible content size guard.

(In reply to comment #44)
> (From update of attachment 189217 [details])
> Attachment 189217 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16640112
> 
> New failing tests:
> platform/chromium/fast/repaint/relayout-fixed-position-after-scale.html

The newly added viewport-constrained visible content size guard didn't consider frame scale factor, thus the test failed to detect the visible content size change.
Comment 47 James Robinson 2013-02-20 00:43:18 PST
(In reply to comment #42)
> (From update of attachment 189217 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review
> 
> > Source/WebCore/page/Page.cpp:751
> > +    view->visibleContentScaleFactorDidChange();
> 
> this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same?

Still waiting for an answer to ^^
Comment 48 James Robinson 2013-02-20 00:44:34 PST
(In reply to comment #43)
> (In reply to comment #42)
> > (From update of attachment 189217 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review
> > 
> > > Source/WebCore/page/Page.cpp:751
> > > +    view->visibleContentScaleFactorDidChange();
> > 
> > this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same?
> 
> FrameView::visibleContentsResized() gets called a lot. Scrolling without changing scrollbar visibility? Still get called. Changing overlay scrollbar visibility? Still get called.
> 
> I would say we'd better apply the same guard on the whole function, but I don't know what are the possible side effects.

I don't really understand this comment.  It seems to be a general statement and not an answer to my question re page scale changes.
Comment 49 Tien-Ren Chen 2013-02-20 01:30:17 PST
Comment on attachment 189217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review

>>>>> Source/WebCore/page/Page.cpp:751
>>>>> +    view->visibleContentScaleFactorDidChange();
>>>> 
>>>> this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same?
>>> 
>>> FrameView::visibleContentsResized() gets called a lot. Scrolling without changing scrollbar visibility? Still get called. Changing overlay scrollbar visibility? Still get called.
>>> 
>>> I would say we'd better apply the same guard on the whole function, but I don't know what are the possible side effects.
>> 
>> Still waiting for an answer to ^^
> 
> I don't really understand this comment.  It seems to be a general statement and not an answer to my question re page scale changes.

For example, when you scroll a page, although visible content size stays the same, but FrameView::visibleContentsResized() will still get called by ScrollView::updateScrollbars().
Check on the visible viewport size is not really a necessity, but an optimization. I can remove that if you want.

If your question is why not just call setViewportConstrainedObjectsNeedLayout() directly, I'd say it is better to avoid code duplication. If fixed-position elements need layout if and only if visible content size changes, then the only place we should do that is in FrameView::visibleContentsResized().
Comment 50 James Robinson 2013-02-20 17:58:43 PST
(In reply to comment #49)
> (From update of attachment 189217 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review
> 
> >>>>> Source/WebCore/page/Page.cpp:751
> >>>>> +    view->visibleContentScaleFactorDidChange();
> >>>> 
> >>>> this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same?
> >>> 
> >>> FrameView::visibleContentsResized() gets called a lot. Scrolling without changing scrollbar visibility? Still get called. Changing overlay scrollbar visibility? Still get called.
> >>> 
> >>> I would say we'd better apply the same guard on the whole function, but I don't know what are the possible side effects.
> >> 
> >> Still waiting for an answer to ^^
> > 
> > I don't really understand this comment.  It seems to be a general statement and not an answer to my question re page scale changes.
> 
> For example, when you scroll a page, although visible content size stays the same, but FrameView::visibleContentsResized() will still get called by ScrollView::updateScrollbars().
> Check on the visible viewport size is not really a necessity, but an optimization. I can remove that if you want.

Please do

> 
> If your question is why not just call setViewportConstrainedObjectsNeedLayout() directly, I'd say it is better to avoid code duplication. If fixed-position elements need layout if and only if visible content size changes, then the only place we should do that is in FrameView::visibleContentsResized().

No, I mean just calling visibleContentsResized() when the page scale factor changed.
Comment 51 James Robinson 2013-02-20 17:59:55 PST
Comment on attachment 189224 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189224&action=review

> Source/WebCore/page/FrameView.cpp:2040
> +    LayoutSize viewportConstrainedVisibleContentSize = viewportConstrainedVisibleContentRect().size();
> +    viewportConstrainedVisibleContentSize.scale(1 / frame()->frameScaleFactor());
> +    if (fixedElementsLayoutRelativeToFrame() && m_lastSeenViewportConstrainedVisibleContentsSize != viewportConstrainedVisibleContentSize)

nuke the guards

> Source/WebCore/page/FrameView.cpp:2042
> +    m_lastSeenViewportConstrainedVisibleContentsSize = viewportConstrainedVisibleContentSize;

don't need this

> Source/WebCore/page/FrameView.cpp:2882
> +void FrameView::visibleContentScaleFactorDidChange()
> +{
> +    if (!m_frame || !m_frame->page())
> +        return;
> +
> +    if (!m_frame->settings()->applyPageScaleFactorInCompositor() || m_frame != m_frame->page()->mainFrame())
> +        return;
> +
> +    visibleContentsResized();

nuke this

> Source/WebCore/page/FrameView.h:186
> +    void visibleContentScaleFactorDidChange();

don't need this

> Source/WebCore/page/FrameView.h:592
> +    // For fixedElementsLayoutRelativeToFrame() == true.
> +    // We don't want unnecessary layout when visible contents size didn't actually change.
> +    LayoutSize m_lastSeenViewportConstrainedVisibleContentsSize;

don't need this

> Source/WebCore/page/Page.cpp:751
> +    view->visibleContentScaleFactorDidChange();

just view->visibleContentsResized()
Comment 52 Tien-Ren Chen 2013-02-20 23:24:11 PST
Created attachment 189463 [details]
Patch
Comment 53 WebKit Review Bot 2013-02-21 09:33:18 PST
Comment on attachment 189463 [details]
Patch

Clearing flags on attachment: 189463

Committed r143616: <http://trac.webkit.org/changeset/143616>
Comment 54 WebKit Review Bot 2013-02-21 09:33:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 55 WebKit Review Bot 2013-02-21 10:46:15 PST
Re-opened since this is blocked by bug 110491
Comment 56 Tien-Ren Chen 2013-02-21 17:36:41 PST
Created attachment 189646 [details]
Patch
Comment 57 Tien-Ren Chen 2013-02-21 17:38:08 PST
Comment on attachment 189646 [details]
Patch

Changes from last patch:
* Adding back the fixedElementsLayoutRelativeToFrame() guard. Unnecessary setViewportConstrainedObjectsNeedLayout() causes weird side-effects on chromium-mac.
Comment 58 James Robinson 2013-02-21 17:45:47 PST
In https://bugs.webkit.org/show_bug.cgi?id=110491#c5 you said:

"Very weird. In this particular test, the frame reserved space for a vertical scrollbar (resizing the RenderView to 785x600) when it doesn't need to. This leaves a gray background (which doesn't look like scrollbar) on the right hand side of the screen, with a shadow near document boundary.

I can avoid triggering this in my patch, but it feels like a bug to me."

I agree that this is probably and bug and we should find it and fix it.
Comment 59 Tien-Ren Chen 2013-02-27 18:03:56 PST
Created attachment 190632 [details]
Patch
Comment 60 Tien-Ren Chen 2013-02-27 18:09:02 PST
Comment on attachment 190632 [details]
Patch

Changes from last patch:
* Revert the change in FrameView::visibleContentsResized

It turns out we can't use FrameView::visibleContentsResized after all. This function will be reached during FrameView::layout from this calling chain:

FrameView::layout
  ScrollView::setScrollbarMode
    ScrollView::updateScrollbars
      FrameView::visibleContentsResized

And this breaks the render tree invariant that when FrameView::layout() ends, no RenderObjects within that frame should need layout. The chromium-mac svg/foreignObject/fixed-position.svg failure is another side-effect of the change.
Comment 61 WebKit Review Bot 2013-02-27 21:07:26 PST
Comment on attachment 190632 [details]
Patch

Clearing flags on attachment: 190632

Committed r144260: <http://trac.webkit.org/changeset/144260>
Comment 62 WebKit Review Bot 2013-02-27 21:07:34 PST
All reviewed patches have been landed.  Closing bug.