[webkit-reviews] review denied: [Bug 80242] REGRESSION(r106232): The resize handler is always called after loading : [Attachment 130618] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Allan Sandfeld
Jensen <allan.jensen at nokia.com>'s request for review:
Bug 80242: REGRESSION(r106232): The resize handler is always called after
loading
https://bugs.webkit.org/show_bug.cgi?id=80242

Attachment 130618: Patch
https://bugs.webkit.org/attachment.cgi?id=130618&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130618&action=review


> Source/WebCore/page/FrameView.cpp:1046
> +		       m_lastViewportSize = visibleContentRect(true
/*includeScrollbars*/).size();

This is not correct for the fixedlayout case:

 230 IntRect ScrollView::visibleContentRect(bool includeScrollbars) const
 231 {
 232	 if (platformWidget())
 233	     return platformVisibleContentRect(includeScrollbars);
 234 
 235	 if (!m_fixedVisibleContentRect.isEmpty())
 236	     return m_fixedVisibleContentRect;

Thus, this returns the visible content rect in CSS coordinates, and thus it
will change every time the scale changes.

It also feels like you are abusing the method for something it was not designed
for. We really want the layout sizes here and not what is visible (even if
those might be the same in the desktop case)


Why not do LayoutSize(layoutWidth() + verticalScrollbar()->width(),
layoutHeight() +  horizontalScrollbar()->height()); ?

> Source/WebCore/page/FrameView.cpp:2332
> +	   // Resize events are not emitted when layoutsize changes due to
added scrollbars, but are emitted if
> +	   // the viewport is externally resized even if the resize does not
affect layoutsize.

Due to compatibility with other browsers, we should not emit resize events when
the layout size changes
due to scrollbars being added or removed; and thus only when the viewport is
externally resized.