Bug 113829 – [BlackBerry] Decouple layout viewport from visual viewport

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Arvid Nilsson]

Setting the visual viewport will force the layout viewport equal to the visual viewport. Decouple by allowing the user to specify a layout viewport (which could be the exact same value they pass for visual viewport, if that's their cup of tea).

PR #318757

[Arvid Nilsson]

Created attachment 196237 [details]
Patch

[Mike Lattanzio]

Looks good.

[Carlos Garcia Campos]

Comment on attachment 196237 [details]
Patch

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

Looks good to me in general, but patch doesn't seem to apply.

> Source/WebKit/blackberry/Api/WebPage.cpp:3915
> +        // If the viewport didn't change, try to apply only the new default layout size

Comments should finish with a period.

> Source/WebKit/blackberry/Api/WebPage.cpp:3945
> +    static ViewportArguments defaultViewportArguments;
> +    if (d->m_viewportArguments != defaultViewportArguments) {
> +        setVirtualViewportSize(d->recomputeVirtualViewportFromViewportArguments());
> +        needsLayout = true;

recomputeVirtualViewportFromViewportArguments already checks whether viewport arguments have changed, so you could avoid checking it twice, by checking if the returned rectangle is empty.

> Source/WebKit/blackberry/Api/WebPage.h:144
> +    // Returns the size of the visual viewport

Ditto.

> Source/WebKit/blackberry/Api/WebPage.h:147
> +    // Sets the sizes of the visual viewport and the layout viewport

Ditto.

> Source/WebKit/blackberry/Api/WebPage.h:153
> +    // Returns the size of the layout viewport

Ditto.

[Arvid Nilsson]

Comment on attachment 196237 [details]
Patch

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

>> Source/WebKit/blackberry/Api/WebPage.cpp:3945
>> +        needsLayout = true;
> 
> recomputeVirtualViewportFromViewportArguments already checks whether viewport arguments have changed, so you could avoid checking it twice, by checking if the returned rectangle is empty.

This is a great idea, to avoid duplicating the ugly "static ViewportArguments defaultViewportArguments" line all over the place.

[Arvid Nilsson]

Created attachment 196295 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 196295 [details]
Patch

Looks great.

[WebKit Review Bot]

Comment on attachment 196295 [details]
Patch

Clearing flags on attachment: 196295

Committed r147537: <http://trac.webkit.org/changeset/147537>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Arvid Nilsson]

Thanks for the review, I found one more place where we can apply the idea and get rid of static ViewportArguments defaultViewportArguments;.