Bug 95964

Created attachment 173389 [details] WIP This patch passes most of the tests submitted by Opera (31 out of 34) on Qt and EFL. Needs a lot of cleanup though.

Attachment 173389 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/css/ViewportStyleResolver.cpp:75: Omit int when using unsigned [runtime/unsigned] [1] Source/WebCore/css/ViewportStyleResolver.cpp:156: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 173389 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=173389&action=review > Source/WebCore/css/RuleSet.cpp:353 > + else if (rule->isViewportRule() && resolver) { > + if (scope) > + continue; other if (scope) have comments. Maybe one would be good here > Source/WebCore/css/ViewportStyleResolver.cpp:77 > + // FIXME: This is wrong. > + for (unsigned int i = 0; i < propertySet->propertyCount(); i++) > + m_propertySet->addParsedProperty(propertySet->propertyAt(i).toCSSProperty()); > +} Bad comment, why is it wrong? :) btw it is "unsigned" and not "unsigned int" > Source/WebCore/css/ViewportStyleResolver.h:67 > + float m_initialViewportWidth; > + float m_initialViewportHeight; FloatSize ? > Source/WebCore/dom/ViewportArguments.cpp:47 > +static float maxIgnoreAuto(const float value1, const float value2) Ignoring? > Source/WebCore/dom/ViewportArguments.cpp:112 > } > > + if (args.type == ViewportArguments::CSSDeviceAdaptation) { > + switch (int(args.minWidth)) { > + case ViewportArguments::ValueDeviceWidth: > + args.minWidth = desktopWidth; > + break; We need to refactor ViewportArguments into a proper class. I suggest renaming it to ViewportDescription > Source/WebCore/dom/ViewportArguments.h:90 > , userScalable(ValueAuto) userZoom? Can be another patch > Source/WebCore/dom/ViewportArguments.h:114 > && maximumScale == other.maximumScale > && width == other.width We should really rename maximumScale to maxZoom. Could be another patch > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1039 > > - m_viewportSize = size; > - > + m_viewportSize = size; > sendViewportAttributesChanged(); > } separate patch?

Comment on attachment 173389 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=173389&action=review > Source/WebCore/css/StyleResolver.h:251 > + ViewportStyleResolver* viewportStyleResolver() const { return m_viewportStyleResolver.get(); } Should probably not be const as per recent coding style update. > Source/WebCore/css/ViewportStyleResolver.cpp:66 > + if (!propertySet) This null check seems useless. StyleRuleViewport::mutableProperties() already makes the assumption that m_properties is non null by calling in its implementation: if (!m_properties->isMutable()) >> Source/WebCore/css/ViewportStyleResolver.cpp:75 >> + for (unsigned int i = 0; i < propertySet->propertyCount(); i++) > > Omit int when using unsigned [runtime/unsigned] [1] We should use: * "unsigned int" -> "unsigned" * preincrement instead of postincrement * cache propertySet->propertyCount() before the for loop > Source/WebCore/css/ViewportStyleResolver.h:59 > + ViewportStyleResolver(Document*); constructor could use explicit here. > Source/WebCore/dom/Document.h:326 > + void setViewportArguments(const ViewportArguments &viewportArguments) { m_viewportArguments = viewportArguments; } "const ViewportArguments& viewportArguments" is the right coding style I believe (space after the &) > Source/WebCore/page/DOMWindow.idl:348 > + attribute WebKitCSSViewportRuleConstructor WebKitCSSViwportRule; WebKitCSSViwportRule -> WebKitCSSViewportRule ?

Created attachment 173689 [details] WIP v2

Comment on attachment 173689 [details] WIP v2 View in context: https://bugs.webkit.org/attachment.cgi?id=173689&action=review > Source/WebCore/css/ViewportStyleResolver.cpp:53 > + float deviceScaleFactor = 0; why not just = 1? > Source/WebCore/css/ViewportStyleResolver.cpp:57 > + if (deviceScaleFactor && deviceScaleFactor != 1) avoid the first check

Comment on attachment 173689 [details] WIP v2 View in context: https://bugs.webkit.org/attachment.cgi?id=173689&action=review >> Source/WebCore/css/ViewportStyleResolver.cpp:57 >> + if (deviceScaleFactor && deviceScaleFactor != 1) > > avoid the first check Indeed. :)

Created attachment 174701 [details] Patch

Created attachment 174703 [details] Patch

Comment on attachment 174703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174703&action=review > Source/WebCore/css/ViewportStyleResolver.cpp:58 > + if (deviceScaleFactor != 1) > + m_initialViewportSize.scale(1.f / deviceScaleFactor); Why does these have to be divided? The viewport should already be in css units. Since you are not testing this anyway, I would remove it from this patch.

Created attachment 174981 [details] Patch

Comment on attachment 174981 [details] Patch Clearing flags on attachment: 174981 Committed r135163: <http://trac.webkit.org/changeset/135163>

All reviewed patches have been landed. Closing bug.