There are many code that should use canHaveScrollbars() or useFixedLayout() instead of delegatesScrolling().
Created attachment 191404 [details] Patch
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?
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.
Comment on attachment 191404 [details] Patch This patch completely changes how resize events are emitted.
Created attachment 191415 [details] Patch
(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?
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.
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
(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 :)
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.
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.
(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.
Created attachment 191673 [details] Patch
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?
(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?
(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.
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.
Anders, Simon, do you think this is a good change?
iOS uses delegatesScrolling(). We need to be very careful to not break it.
Comment on attachment 191673 [details] Patch r- to clear stale patches from the review queue