Bug 95964 - Apply the resolved viewport rules
Summary: Apply the resolved viewport rules
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago Marcos P. Santos
URL:
Keywords: WebExposed
Depends on:
Blocks: 95959
  Show dependency treegraph
 
Reported: 2012-09-06 03:08 PDT by Thiago Marcos P. Santos
Modified: 2012-11-19 08:37 PST (History)
14 users (show)

See Also:


Attachments
WIP (40.44 KB, patch)
2012-11-09 16:06 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
WIP v2 (30.48 KB, patch)
2012-11-12 12:36 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (140.40 KB, patch)
2012-11-16 08:53 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (140.40 KB, patch)
2012-11-16 09:02 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (140.17 KB, patch)
2012-11-19 07:47 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Marcos P. Santos 2012-09-06 03:08:25 PDT
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.
Comment 1 Thiago Marcos P. Santos 2012-11-09 16:06:25 PST
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.
Comment 2 WebKit Review Bot 2012-11-09 16:09:08 PST
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 3 Kenneth Rohde Christiansen 2012-11-11 05:22:41 PST
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 4 Chris Dumez 2012-11-11 05:43:42 PST
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 ?
Comment 5 Thiago Marcos P. Santos 2012-11-12 12:36:52 PST
Created attachment 173689 [details]
WIP v2
Comment 6 Kenneth Rohde Christiansen 2012-11-12 12:46:41 PST
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 7 Thiago Marcos P. Santos 2012-11-12 14:25:10 PST
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. :)
Comment 8 Thiago Marcos P. Santos 2012-11-16 08:53:37 PST
Created attachment 174701 [details]
Patch
Comment 9 Thiago Marcos P. Santos 2012-11-16 09:02:18 PST
Created attachment 174703 [details]
Patch
Comment 10 Kenneth Rohde Christiansen 2012-11-18 10:35:21 PST
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.
Comment 11 Thiago Marcos P. Santos 2012-11-19 07:47:39 PST
Created attachment 174981 [details]
Patch
Comment 12 WebKit Review Bot 2012-11-19 08:37:42 PST
Comment on attachment 174981 [details]
Patch

Clearing flags on attachment: 174981

Committed r135163: <http://trac.webkit.org/changeset/135163>
Comment 13 WebKit Review Bot 2012-11-19 08:37:48 PST
All reviewed patches have been landed.  Closing bug.