[webkit-reviews] review granted: [Bug 43852] [Qt] resizeToContent seems to trigger infinite resize on some pages : [Attachment 107867] Adding external references to W3C spec in changelog comments + minor fixes

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Adenilson
Cavalcanti Silva <adenilson.silva at openbossa.org>'s request for review:
Bug 43852: [Qt] resizeToContent seems to trigger infinite resize on some pages
https://bugs.webkit.org/show_bug.cgi?id=43852

Attachment 107867: Adding external references to W3C spec in changelog comments
+ minor fixes
https://bugs.webkit.org/attachment.cgi?id=107867&action=review

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


r=me as the code is fine and it passes tests. I am cq-ing though as it is an
important change and I want the changelog to be as clear as possible and to the
point. Good work though!

> Source/WebCore/ChangeLog:19
> +	   Inner height and width are now calculated using
WebCore::ScrollView::visibleContentRect
> +	   including the scrollbars (if there are any).
> +
> +	   Referring the W3C spec "CSSOM View Module", section "Extensions to
> +	   the Window Interface":
> +	   - "... innerWidth attribute must return the viewport width
> +	   including the size of a rendered scroll bar (if any)."
> +	   - "The innerHeight attribute must return the viewport height
> +	   including the size of a rendered scroll bar (if any)."
> +
> +	   It will now return correct values when using tiling backing store
> +	   thus preventing infinite resize events.

I would make this more to the point, so that the change is easy to understand
for most people.:


InnerHeight and -Width are now calculated using
ScrollView::visibleContentRect() including scrollbars (if any), instead of
using ScrollView::frameRect() as before.

This makes no behavior change when not using the tiled backing store and is
compliant with the definition stated in the CSSOM View Module. The change was
made so that we would be returning the correct values when using the tiled
backing store and thus fix the original bug report, which was to avoid infinite
resize events caused by wrong innerHeight and -Width values.

> Source/WebCore/ChangeLog:25
> +
> +

Please remove double newlines before committing

> Source/WebCore/page/DOMWindow.cpp:1096
> +    return static_cast<int>(view->visibleContentRect(/* include the
scrollbars */ true).height() / m_frame->pageZoomFactor());

This is fine, but it is more common to just write the original method argument.
ie. includeScrollbars or whatever it is called.