Bug 90286 - [EFL] [GTK] [QT] fast/viewport/viewport-91.html is failing after r121555
Summary: [EFL] [GTK] [QT] fast/viewport/viewport-91.html is failing after r121555
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Konrad Piascik
URL:
Keywords:
: 90287 (view as bug list)
Depends on: 90376
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-29 08:20 PDT by Sudarsana Nagineni (babu)
Modified: 2012-07-02 05:42 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.26 KB, patch)
2012-06-29 13:27 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch (4.24 KB, patch)
2012-06-29 13:52 PDT, Konrad Piascik
no flags Details | Formatted Diff | Diff
Patch for landing (4.19 KB, patch)
2012-07-01 11:22 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (4.80 KB, patch)
2012-07-01 23:57 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sudarsana Nagineni (babu) 2012-06-29 08:20:31 PDT
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
Comment 1 Sudarsana Nagineni (babu) 2012-06-29 09:02:46 PDT
Skipping test for GTK, EFL and QT ports in bug 90287.
Comment 2 Konrad Piascik 2012-06-29 13:27:53 PDT
Created attachment 150247 [details]
Patch
Comment 3 Daniel Bates 2012-06-29 13:42:32 PDT
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>.
Comment 4 Konrad Piascik 2012-06-29 13:52:26 PDT
Created attachment 150252 [details]
Patch
Comment 5 WebKit Review Bot 2012-06-29 19:38:33 PDT
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
Comment 6 Adam Barth 2012-07-01 10:07:28 PDT
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.
Comment 7 Konrad Piascik 2012-07-01 10:36:16 PDT
(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>
Comment 8 Adam Barth 2012-07-01 11:16:54 PDT
> 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.
Comment 9 Adam Barth 2012-07-01 11:22:02 PDT
Created attachment 150332 [details]
Patch for landing
Comment 10 Adam Barth 2012-07-01 11:23:17 PDT
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.
Comment 11 WebKit Review Bot 2012-07-01 14:01:51 PDT
Comment on attachment 150332 [details]
Patch for landing

Clearing flags on attachment: 150332

Committed r121635: <http://trac.webkit.org/changeset/121635>
Comment 12 WebKit Review Bot 2012-07-01 14:01:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 James Robinson 2012-07-01 19:52:40 PDT
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...
Comment 14 James Robinson 2012-07-01 19:56:03 PDT
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>
Comment 15 Adam Barth 2012-07-01 23:57:07 PDT
Created attachment 150365 [details]
Patch for landing
Comment 16 Adam Barth 2012-07-01 23:57:26 PDT
This one should build on clang.
Comment 17 Balazs Kelemen 2012-07-02 00:35:39 PDT
Comment on attachment 150365 [details]
Patch for landing

Clearing flags on attachment: 150365

Committed r121661: <http://trac.webkit.org/changeset/121661>
Comment 18 Balazs Kelemen 2012-07-02 00:35:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Balazs Kelemen 2012-07-02 00:38:44 PDT
*** Bug 90287 has been marked as a duplicate of this bug. ***
Comment 20 Csaba Osztrogonác 2012-07-02 05:25:21 PDT
(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