Bug 95964 – Apply the resolved viewport rules

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Thiago Marcos P. Santos]

At this point, we should resolve all the constraints, cascading, etc. The initial scaled, max and min zoom levels, etc are signaled and reflected on the UI.

[Thiago Marcos P. Santos]

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.

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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?

[Chris Dumez]

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 ?

[Thiago Marcos P. Santos]

Created attachment 173689 [details]
WIP v2

[Kenneth Rohde Christiansen]

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

[Thiago Marcos P. Santos]

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. :)

[Thiago Marcos P. Santos]

Created attachment 174701 [details]
Patch

[Thiago Marcos P. Santos]

Created attachment 174703 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[Thiago Marcos P. Santos]

Created attachment 174981 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174981 [details]
Patch

Clearing flags on attachment: 174981

Committed r135163: <http://trac.webkit.org/changeset/135163>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.