Bug 113829

Summary: [BlackBerry] Decouple layout viewport from visual viewport
Product: WebKit Reporter: Arvid Nilsson <anilsson>
Component: WebKit BlackBerryAssignee: Arvid Nilsson <anilsson>
Status: RESOLVED FIXED    
Severity: Normal CC: anilsson, cgarcia, jkjiang, jpetsovits, mifenton, mlattanzio, rwlbuis, tonikitoo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Arvid Nilsson 2013-04-02 15:10:17 PDT 
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
Comment 1 Arvid Nilsson 2013-04-02 15:20:00 PDT 
Created attachment 196237 [details]
Patch
Comment 2 Mike Lattanzio 2013-04-02 15:24:28 PDT 
Looks good.
Comment 3 Carlos Garcia Campos 2013-04-03 00:17:50 PDT 
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.
Comment 4 Arvid Nilsson 2013-04-03 01:01:42 PDT 
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.
Comment 5 Arvid Nilsson 2013-04-03 01:04:21 PDT 
Created attachment 196295 [details]
Patch
Comment 6 Carlos Garcia Campos 2013-04-03 01:08:37 PDT 
Comment on attachment 196295 [details]
Patch

Looks great.
Comment 7 WebKit Review Bot 2013-04-03 04:04:51 PDT 
Comment on attachment 196295 [details]
Patch

Clearing flags on attachment: 196295

Committed r147537: <http://trac.webkit.org/changeset/147537>
Comment 8 WebKit Review Bot 2013-04-03 04:04:55 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 9 Arvid Nilsson 2013-04-03 06:16:21 PDT 
Thanks for the review, I found one more place where we can apply the idea and get rid of static ViewportArguments defaultViewportArguments;.