Bug 111408 – [WK2] Use canHaveScrollbars() or useFixedLayout() instead of delegatesScrolling().

• Component: Layout and Rendering
• Status: NEW

[Dongseong Hwang]

There are many code that should use canHaveScrollbars() or useFixedLayout() instead of delegatesScrolling().

[Dongseong Hwang]

Created attachment 191404 [details]
Patch

[Dongseong Hwang]

Currently, only Qt and EFL set delegatesScrolling to true. And Qt and EFL set canHaveScrollbars to false. So Qt and EFL are fine with this patch.

But I'm afraid that the outer ports which set canHaveScrollbars to false would have problem. Could you let me know what port set canHaveScrollbars to false?

[Allan Sandfeld Jensen]

Comment on attachment 191404 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1198
> -                    if (useFixedLayout() && !fixedLayoutSize().isEmpty() && delegatesScrolling())
> -                        m_lastViewportSize = fixedLayoutSize();
> -                    else
> +                    if (canHaveScrollbars())
>                          m_lastViewportSize = visibleContentRect(IncludeScrollbars).size();
> +                    else
> +                        m_lastViewportSize = visibleContentRect(ExcludeScrollbars).size();

visibleContentRetc(ExcludeScrollbars) is the same as fixedVisibleRect, not the same as fixedLayoutSize.

[Allan Sandfeld Jensen]

Comment on attachment 191404 [details]
Patch

This patch completely changes how resize events are emitted.

[Dongseong Hwang]

Created attachment 191415 [details]
Patch

[Dongseong Hwang]

(In reply to comment #3)
> (From update of attachment 191404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191404&action=review
> visibleContentRetc(ExcludeScrollbars) is the same as fixedVisibleRect, not the same as fixedLayoutSize.

Thank you for review. you are right. I made a mistake.
How about the second patch?

[Allan Sandfeld Jensen]

Comment on attachment 191415 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2648
> +        if (useFixedLayout() && !fixedLayoutSize().isEmpty() && useFixedLayout())

useFixedLayout() && useFixedLayout()?

I can remember the reason delegatesScrolling() was used was to ensure it only affected the right platforms. You may want to try to run blame on the lines and dig up the bug where we discussed it.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2227
> -    if (m_renderView->frameView()->delegatesScrolling())
> +    if (!m_renderView->frameView()->canHaveScrollbars())

This is wrong. CanHaveScrollbars can be set by simply forcing scrollbars to be disabled (ScrollbarAlwaysOff). It does not imply delegated scrolling or paintsEntireContents. For instance without a scrollbar, you could still scroll using scroll gestures, mouse wheel or JavaScript events.

[WebKit Review Bot]

Comment on attachment 191415 [details]
Patch

Attachment 191415 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17029021

New failing tests:
compositing/iframes/nested-composited-iframe.html
compositing/iframes/overlapped-nested-iframes.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-iframe-fixed.html
platform/chromium/virtual/softwarecompositing/framesets/composited-frame-alignment.html
compositing/framesets/composited-frame-alignment.html
compositing/iframes/invisible-nested-iframe-show.html
platform/chromium/virtual/softwarecompositing/iframes/invisible-nested-iframe-show.html
platform/chromium/virtual/softwarecompositing/iframes/overlapped-nested-iframes.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-iframe-absolute.html
platform/chromium/virtual/softwarecompositing/iframes/become-composited-nested-iframes.html
platform/chromium/virtual/softwarecompositing/iframes/nested-composited-iframe.html
compositing/iframes/become-composited-nested-iframes.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-iframe-relative.html

[Dongseong Hwang]

(In reply to comment #7)
> (From update of attachment 191415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191415&action=review
Thanks for explanation. I spit it to get this valuable explanation. thanks again. I'll survey more and ask questions :)

[Jocelyn Turcotte]

Comment on attachment 191415 [details]
Patch

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 191415 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191415&action=review
> Thanks for explanation. I spit it to get this valuable explanation. thanks again. I'll survey more and ask questions :)

Please ask for explanations on IRC, proposing faulty patches to learn is like skipping classes and taking exams until you get it right enough to get a pass grade.

[Simon Fraser (smfr)]

Comment on attachment 191415 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        There are many code that should use canHaveScrollbars() or useFixedLayout()
> +        instead of delegatesScrolling().

"There are many parts of the code" or "There is much code".

This should explain why using delegatesScrolling() is wrong.

[Dongseong Hwang]

(In reply to comment #10)
> (From update of attachment 191415 [details])
> (In reply to comment #9)
> Please ask for explanations on IRC, proposing faulty patches to learn is like skipping classes and taking exams until you get it right enough to get a pass grade.

Ok, I'll do it. Sorry for skipping classes. see yon in IRC.

(In reply to comment #11)
> (From update of attachment 191415 [details])
> This should explain why using delegatesScrolling() is wrong.

I agree. I'll support.

[Dongseong Hwang]

Created attachment 191673 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 191673 [details]
Patch

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

> Source/WebKit2/ChangeLog:17
> +        1. WebChromeClient::contentsSizeChanged() uses canHaveScrollbars().
> +        In theory, it is possible for FrameView to have scrollbars although
> +        FrameView delegates scrolling to UI. So it is more natural to check
> +        canHaveScrollbars() before scrollbar code.
> +
> +        2. WebPage::didCompletePageTransition() uses useFixedLayout().
> +        EFL and Qt uses PageViewportController that receives
> +        PageTransitionViewportReady massages when using fixed layout, so it is
> +        more natural to check useFixedLayout().
> +

can't this be tested?

[Dongseong Hwang]

(In reply to comment #13)
> Created an attachment (id=191673) [details]
> Patch

I remain only WK2 part.

(In reply to comment #7)
> (From update of attachment 191415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191415&action=review
> I can remember the reason delegatesScrolling() was used was to ensure it only affected the right platforms. You may want to try to run blame on the lines and dig up the bug where we discussed it.

I studied the history, and now I understand what is layout viewport and visual viewport. (I added the links that helped you 1 years ago in Reference of https://trac.webkit.org/wiki/ScalesAndZooms)
And I posted Bug 11532 to clean up a little on your great work. could you take a look?

[Dongseong Hwang]

(In reply to comment #14)
> (From update of attachment 191673 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191673&action=review
> can't this be tested?

only in EFL and Qt. I does not have mac book. I'm sorry for Mac.

[Dongseong Hwang]

Comment on attachment 191673 [details]
Patch

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

>> Source/WebKit2/ChangeLog:17
>> +
> 
> can't this be tested?

I think it is hard to test, because this patch does not change behavior actually.

1. WebChromeClient::contentsSizeChanged() uses canHaveScrollbars().
It does not change any WK2 internal behavior. This change only affect whether WKPageHasHorizontalScrollbar can return true or not.

2. WebPage::didCompletePageTransition() uses useFixedLayout().
This routine is used by only EFL and Qt because there is USE(TILED_BACKING_STORE) guard. And when we enable/disable useFixedLayout, delegatesScrolling always is enabled/disabled.

[Darin Adler]

Anders, Simon, do you think this is a good change?

[Simon Fraser (smfr)]

iOS uses delegatesScrolling(). We need to be very careful to not break it.

[Brady Eidson]

Comment on attachment 191673 [details]
Patch

r- to clear stale patches from the review queue