Bug 90286 – [EFL] [GTK] [QT] fast/viewport/viewport-91.html is failing after r121555

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Sudarsana Nagineni (babu)]

fast/viewport/viewport-91.html started failing after r121555

Here is the diff:

--- /home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/layout-test-results/fast/viewport/viewport-91-expected.txt
+++ /home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/layout-test-results/fast/viewport/viewport-91-actual.txt
@@ -1,2 +1,2 @@
-viewport size 320x356 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
+viewport size 480x534 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000

http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug/r121559%20%282148%29/fast/viewport/viewport-91-diff.txt

[Sudarsana Nagineni (babu)]

Skipping test for GTK, EFL and QT ports in bug 90287.

[Konrad Piascik]

Created attachment 150247 [details]
Patch

[Daniel Bates]

Comment on attachment 150247 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150247&action=review

> Source/WebCore/dom/ViewportArguments.h:113
> +    static const float deprecatedTargetDPI = 160.0;

It's unnecessary to add the suffix ".0" to the value of this float given its static type. Removing the suffix would also make this definition consist with the Code Style Guidelines, <http://www.webkit.org/coding/coding-style.html#float-suffixes>.

[Konrad Piascik]

Created attachment 150252 [details]
Patch

[WebKit Review Bot]

Comment on attachment 150252 [details]
Patch

Rejecting attachment 150252 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Dan Bates found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13109582

[Adam Barth]

Comment on attachment 150252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150252&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by Dan Bates

I think you're missing the . at the end.

[Konrad Piascik]

(In reply to comment #6)
> (From update of attachment 150252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150252&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by Dan Bates
> 
> I think you're missing the . at the end.

Is this really a requirement of the style checker? If so I suggest it be removed or that check-webkit-style and webkit-patch be updated tonotify users of the error. Something like this should not prevent a fix from going in imho. </rant>

[Adam Barth]

> Is this really a requirement of the style checker? If so I suggest it be removed or that check-webkit-style and webkit-patch be updated tonotify users of the error. Something like this should not prevent a fix from going in imho. </rant>

Patches welcome.  :)

It's just a regexp in this file:

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/changelog.py

It probably should be tweaked a bit.

[Adam Barth]

Created attachment 150332 [details]
Patch for landing

[Adam Barth]

Comment on attachment 150252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150252&action=review

>>> Source/WebCore/ChangeLog:6
>>> +        Reviewed by Dan Bates
>> 
>> I think you're missing the . at the end.
> 
> Is this really a requirement of the style checker? If so I suggest it be removed or that check-webkit-style and webkit-patch be updated tonotify users of the error. Something like this should not prevent a fix from going in imho. </rant>

Actually, I think the problem might be that he's listed as Daniel rather than Dan.  We have a fuzzy matcher for names, maybe we should use that rather than an exact match.  It's slightly tricky because it's important to make sure that we know who reviewed each patch, but I'm sure there's some way of making it work.

[WebKit Review Bot]

Comment on attachment 150332 [details]
Patch for landing

Clearing flags on attachment: 150332

Committed r121635: <http://trac.webkit.org/changeset/121635>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This doesn't appear to compile on clang mac:


In file included from ../../Source/WebCore/dom/Document.h:49:
../../Source/WebCore/dom/ViewportArguments.h:113:24: error: in-class initializer for static data member of type 'const float' is a GNU extension [-Werror,-Wgnu]
    static const float deprecatedTargetDPI = 160;

Apple Lion Debug (Build): compiled failed -  stdio
Apple Win Debug (Build): compiled failed -  stdio
Apple Win Release (Build): compiled failed -  stdio
Chromium Mac Release: compiled failed -  stdio

commencing a rollout...

[James Robinson]

Reverted r121635 for reason:

Breaks compile on clang error: in-class initializer for static data member of type 'const float' is a GNU extension [-Werror,-Wgnu]

Committed r121651: <http://trac.webkit.org/changeset/121651>

[Adam Barth]

Created attachment 150365 [details]
Patch for landing

[Adam Barth]

This one should build on clang.

[Balazs Kelemen]

Comment on attachment 150365 [details]
Patch for landing

Clearing flags on attachment: 150365

Committed r121661: <http://trac.webkit.org/changeset/121661>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

*** Bug 90287 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

(In reply to comment #17)
> (From update of attachment 150365 [details])
> Clearing flags on attachment: 150365
> 
> Committed r121661: <http://trac.webkit.org/changeset/121661>

This test still fail on Qt-WK2:

--- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/viewport/viewport-91-expected.txt
+++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/viewport/viewport-91-actual.txt
@@ -1,2 +1,2 @@
-viewport size 320x356 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
+viewport size 480x534 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000