Bug 103090 - [WK2] TiledBackingStore: page contents is scaled wrongly
Summary: [WK2] TiledBackingStore: page contents is scaled wrongly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on: 102922
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-22 13:57 PST by Mikhail Pozdnyakov
Modified: 2012-11-26 13:19 PST (History)
6 users (show)

See Also:


Attachments
patch (5.14 KB, patch)
2012-11-22 14:13 PST, Mikhail Pozdnyakov
kenneth: review-
Details | Formatted Diff | Diff
patch v2 (8.23 KB, patch)
2012-11-25 04:59 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2012-11-22 13:57:51 PST
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 :)
Comment 1 Mikhail Pozdnyakov 2012-11-22 14:13:40 PST
Created attachment 175710 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2012-11-22 15:13:06 PST
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
Comment 3 Mikhail Pozdnyakov 2012-11-25 04:59:44 PST
Created attachment 175881 [details]
patch v2

Took Kenneth feedback into consideration. Added test.
Comment 4 Kenneth Rohde Christiansen 2012-11-25 06:53:31 PST
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.
Comment 5 WebKit Review Bot 2012-11-25 10:30:59 PST
Comment on attachment 175881 [details]
patch v2

Clearing flags on attachment: 175881

Committed r135669: <http://trac.webkit.org/changeset/135669>
Comment 6 WebKit Review Bot 2012-11-25 10:31:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Thiago Marcos P. Santos 2012-11-26 02:17:08 PST
Funny that after this patch,

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

and

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

stopped passing.
Comment 8 Mikhail Pozdnyakov 2012-11-26 02:50:57 PST
(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.
Comment 9 Thiago Marcos P. Santos 2012-11-26 12:08:06 PST
(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));
Comment 10 Mikhail Pozdnyakov 2012-11-26 13:11:37 PST
(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.