Bug 113829 - [BlackBerry] Decouple layout viewport from visual viewport
Summary: [BlackBerry] Decouple layout viewport from visual viewport
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arvid Nilsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-02 15:10 PDT by Arvid Nilsson
Modified: 2013-04-03 06:16 PDT (History)
10 users (show)

See Also:


Attachments
Patch (7.42 KB, patch)
2013-04-02 15:20 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff
Patch (7.20 KB, patch)
2013-04-03 01:04 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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;.