Bug 103090

Created attachment 175710 [details] patch

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

Created attachment 175881 [details] patch v2 Took Kenneth feedback into consideration. Added test.

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 on attachment 175881 [details] patch v2 Clearing flags on attachment: 175881 Committed r135669: <http://trac.webkit.org/changeset/135669>

All reviewed patches have been landed. Closing bug.

Funny that after this patch, css3/device-adapt/opera/constrain-006.xhtml and css3/device-adapt/opera/constrain-007.xhtml stopped passing.

(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.

(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));

(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.