Bug 103090 – [WK2] TiledBackingStore: page contents is scaled wrongly

• Component: WebKit2
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

Currently the page scaling is defined by m_rawAttributes.initialScale in PageViewportController. If initial scale was not specified in the viewport meta tag it's set to m_minimumScaleToFit in PageViewportController::didChangeViewportAttributes(). Problem is that 
m_minimumScaleToFit can have wrong value as contents size might not be updated at the time PageViewportController::didChangeViewportAttributes() is invoked. 

Here is steps to reproduce wrong scaling:
1) open http://people.opera.com/andreasb/viewport/ex01.html
2) go to any site w/o viewport meta tag (for example google.com)
3) you will see it :)

[Mikhail Pozdnyakov]

Created attachment 175710 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 175710 [details]
patch

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

> Source/WebKit2/ChangeLog:16
> +
> +        Before this change the page contents scaling in PageViewportController was defined all the times
> +        by m_rawAttributes.initialScale. If initial scale had not been specified in the viewport meta tag
> +        it was set to m_minimumScaleToFit inside PageViewportController::didChangeViewportAttributes().
> +        The problem was that m_minimumScaleToFit could have wrong value as contents size might have not be updated
> +        at the time PageViewportController::didChangeViewportAttributes() was invoked.
> +        The solution is to use m_minimumScaleToFit for contents scaling if initial scale is not specified in the viewport meta tag,
> +        as it is updated all the time. Also a flag m_initiallyFitToViewport is added to PageViewportController
> +        to detect whether m_minimumScaleToFit should be used for scaling.
> +

Could you use shorter lines, some newlines etc to make this easier to read? Also doesn't this affect css @viewport as well?

Why don't we add a test?

> Source/WebKit2/UIProcess/PageViewportController.cpp:111
> -{
> +{    

That seems wrong

[Mikhail Pozdnyakov]

Created attachment 175881 [details]
patch v2

Took Kenneth feedback into consideration. Added test.

[Kenneth Rohde Christiansen]

Comment on attachment 175881 [details]
patch v2

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

> LayoutTests/ChangeLog:13
> +        * css3/device-adapt/resources/check-contents-width.html: Added.
> +        * css3/device-adapt/viewport-width-not-affecting-next-page-expected.txt: Added.
> +        * css3/device-adapt/viewport-width-not-affecting-next-page.html: Added.

Great, now it would be good with tests for "fit to width" as well, especially pages setting a size, say width 980 and they grows beyond that.

[WebKit Review Bot]

Comment on attachment 175881 [details]
patch v2

Clearing flags on attachment: 175881

Committed r135669: <http://trac.webkit.org/changeset/135669>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

Funny that after this patch,

css3/device-adapt/opera/constrain-006.xhtml

and

css3/device-adapt/opera/constrain-007.xhtml

stopped passing.

[Mikhail Pozdnyakov]

(In reply to comment #7)
> Funny that after this patch,
> 
> css3/device-adapt/opera/constrain-006.xhtml
> 
> and
> 
> css3/device-adapt/opera/constrain-007.xhtml
> 
> stopped passing.

Thanks for noticing! will take a look.

[Thiago Marcos P. Santos]

(In reply to comment #8)
> (In reply to comment #7)
> > Funny that after this patch,
> > 
> > css3/device-adapt/opera/constrain-006.xhtml
> > 
> > and
> > 
> > css3/device-adapt/opera/constrain-007.xhtml
> > 
> > stopped passing.
> 
> Thanks for noticing! will take a look.

The patch bellow makes them flaky (but it is obviously not a fix):

--- a/Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp
+++ b/Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp
@@ -59,7 +59,7 @@ PlatformWebView::PlatformWebView(WKContextRef context, WKPageGroupRef pageGroup,
 
     if (m_usingFixedLayout) {
         m_view = toImpl(WKViewCreateWithFixedLayout(evas, context, pageGroup));
-        evas_object_resize(m_view, 800, 600);
+        evas_object_resize(m_view, 980, 600);
     } else
         m_view = toImpl(WKViewCreate(evas, context, pageGroup));

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Funny that after this patch,
> > > 
> > > css3/device-adapt/opera/constrain-006.xhtml
> > > 
> > > and
> > > 
> > > css3/device-adapt/opera/constrain-007.xhtml
> > > 
> > > stopped passing.
> > 
> > Thanks for noticing! will take a look.
> 
> The patch bellow makes them flaky (but it is obviously not a fix):
> 
> --- a/Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp
> +++ b/Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp
> @@ -59,7 +59,7 @@ PlatformWebView::PlatformWebView(WKContextRef context, WKPageGroupRef pageGroup,
> 
>      if (m_usingFixedLayout) {
>          m_view = toImpl(WKViewCreateWithFixedLayout(evas, context, pageGroup));
> -        evas_object_resize(m_view, 800, 600);
> +        evas_object_resize(m_view, 980, 600);
>      } else
>          m_view = toImpl(WKViewCreate(evas, context, pageGroup));
nope, it shouldn't be a fix. Looks like we have same raise condition caused by IPC I've faced already making tests for this bug, note that given test works fine with mini browser.