[webkit-reviews] review granted: [Bug 67308] [Qt] TouchWebView crashes with segmentation fault : [Attachment 105832] Took care of review suggestions

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Gopal Raghavan
<gopal.1.raghavan at nokia.com>'s request for review:
Bug 67308: [Qt] TouchWebView crashes with segmentation fault
https://bugs.webkit.org/show_bug.cgi?id=67308

Attachment 105832: Took care of review suggestions
https://bugs.webkit.org/attachment.cgi?id=105832&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105832&action=review


Patch looks good but the changelog and the tests can be made better.

> Source/WebKit2/ChangeLog:15
> +	   If you instantiate TouchWebView element with height and width in a
qml file and
> +	   load a url it crashes. During first round
> +	   updateViewportConstraints has non-zero width and 0 as height. As a
result availableSize
> +	   is set to (width,0). Height gets set by subsequent call to
setHeight. However, the
> +	   ASSERT in computeViewportAttributes expects non-zero values for both
height and width.
> +	   setWidth and setHeight are called sequentially therefore it can
happen that computeViewportAttributes
> +	   was called with a size like (width, 0) breaking the assumption of
the function that the size is valid. The patch makes sure we compute the
viewport when both height and width are valid.
> +

Please rewrap this and rephrase it a bit clearer.

"If you instantiate TouchWebView element with height and width in a qml file
and load a url it crashes" -> QTouchWebView can asserts when loading a page if
the viewport does not have a valid geometry.

"setWidth and setHeight" -> The functions setWidth() and setHeight() so that
sentence is a bit easier to read.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/TouchWebView/tst_load.qml:8
> +    height: 600
> +    width: 400

0, 0???