Bug 95557 – [Qt] ASSERT(m_useFixedLayout) in WebPage when using QRawWebView without setting fixed layout

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Balazs Kelemen]

This has been revealed in https://bugs.webkit.org/show_bug.cgi?id=90394.
At first I was thinking that this is simply a matter of a missing call to WebPageProxy::setUseFixedLayout.
But if I made this call, than I could not use qrawwebview for layout tests because it always sets the viewport size to 980x800. I was trying setFixedLayoutSize but it does not help. In WebPage::sendViewportAttributesChanged the given size argument is adjusted in accordance to the viewport tag specification (WebCore::computeViewportAttributes). It is strange to me that calling this function with default desktop arguments (devicewidth/height==viewportSize.width()/height(), args=default, minimumWidth=viewportSize.width(), scale=1, viewportSize=800x600) adjust the size to 980x800 but I don't know the specification.
In order to fix this I see two option:
 1. introduce a setting in WebPage/~Proxy that enables/disables handling the viewport attributes (qrawwebview is missing the UI side of it anyway)
 2. let WTR with qrawwebview run in non fixed layout mode and don't call webpageproxy->setViewportSize.
 3. find out how can we set a custom viewport size in fixed layout mode

3 seems to be the best but I had no success with that, so for now I would like to go with 2.

[Balazs Kelemen]

Created attachment 161665 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 161665 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:334
> +    // Note: it's up to our client to decide if it wants fixed layout.
> +    if (d->m_webPageProxy->useFixedLayout())
> +        d->m_webPageProxy->setViewportSize(size);

whether*. Anyway does that commetn add any value?

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:339
> +        WebKit::LayerTreeCoordinatorProxy* coordinator = drawingArea->layerTreeCoordinatorProxy();
> +        if (!coordinator)
> +            return;
> +        coordinator->setContentsSize(WebCore::FloatSize(size.width(), size.height()));

That could be written in two lines instead of 4

[Balazs Kelemen]

(In reply to comment #2)
> (From update of attachment 161665 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161665&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:334
> > +    // Note: it's up to our client to decide if it wants fixed layout.
> > +    if (d->m_webPageProxy->useFixedLayout())
> > +        d->m_webPageProxy->setViewportSize(size);
> 
> whether*. Anyway does that commetn add any value?

Not much, I can remove it.

> 
> > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:339
> > +        WebKit::LayerTreeCoordinatorProxy* coordinator = drawingArea->layerTreeCoordinatorProxy();
> > +        if (!coordinator)
> > +            return;
> > +        coordinator->setContentsSize(WebCore::FloatSize(size.width(), size.height()));
> 
> That could be written in two lines instead of 4

Than I need an else+return, so it would still be 4 lines :)

[Kenneth Rohde Christiansen]

Comment on attachment 161665 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:339
>>> +        coordinator->setContentsSize(WebCore::FloatSize(size.width(), size.height()));
>> 
>> That could be written in two lines instead of 4
> 
> Than I need an else+return, so it would still be 4 lines :)

Ah you are right

else if (WebKit::LayerTreeCoordinatorProxy* coordinator = drawingArea->layerTreeCoordinatorProxy())
    coordinator->
else
    return;

[Noam Rosenthal]

Comment on attachment 161665 [details]
Patch

Please remove comment before committing as Kenneth suggests.

[Balazs Kelemen]

Landed in http://trac.webkit.org/changeset/127275