Bug 40254

Summary: [Qt] FrameLoaderClientQt.cpp has coding-style errors
Product: WebKit Reporter: Anders Bakken <agbakken>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Trivial CC: abarth, benjamin, commit-queue, eric, jedrzej.nowacki, webkit.review.bot
Priority: P5 Keywords: EasyFix, Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
levin: review-
Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp
none
My previous patch is now deprecated, some mistaken has been done. Check this one instead.
none
Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp none

Description Anders Bakken 2010-06-07 12:04:06 PDT
[Qt] FrameLoaderClientQt.cpp has coding-style errors
Comment 1 Anders Bakken 2010-06-07 12:04:34 PDT
Created attachment 58068 [details]
Patch
Comment 2 David Levin 2010-06-09 09:19:18 PDT
Comment on attachment 58068 [details]
Patch

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:83
 +  static QString drtDescriptionSuitableForTestResult(WebCore::Frame* wframe)
wframe is an abbreviation and WebKit avoids those for variable names. Maybe webCoreFrame would be better.
Comment 3 Jędrzej Nowacki 2010-06-15 06:26:25 PDT
Personally I think that change should be avoided, you are touching most of the lines, it means that when somebody will call git blame he will find your patch on top. It is not end of the world, but it is annoying.

R- for incorect comments. A comment in the webkit should be a full english phrase ended with '.'
Comment 4 Rafael Brandao 2011-04-06 09:23:24 PDT
Created attachment 88445 [details]
Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp
Comment 5 Alexis Menard (darktears) 2011-04-06 09:34:13 PDT
Comment on attachment 88445 [details]
Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp

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

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:47
> +#include "HistoryItem.h"

I'm not sure but this should go before HTMLAppletElement.h

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:91
> +#include <qfileinfo.h>

can you use <QFileInfo> instead and mix it with the other Qt includes?

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1053
> +    // qDebug() << "FrameLoaderClientQt::dispatchWillSendRequest" << request.isNull() << request.url().string`();

What is this `?
Comment 6 Rafael Brandao 2011-04-06 10:11:49 PDT
Created attachment 88460 [details]
My previous patch is now deprecated, some mistaken has been done. Check this one instead.
Comment 7 WebKit Review Bot 2011-04-06 10:13:56 PDT
Attachment 88460 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:60:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:79:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Rafael Brandao 2011-04-06 10:32:29 PDT
Created attachment 88467 [details]
Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp
Comment 9 Kenneth Rohde Christiansen 2011-04-06 15:14:32 PDT
Comment on attachment 88467 [details]
Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp

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

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:97
> +static QString drtDescriptionSuitableForTestResult(WebCore::Frame* webCoreFrame)

We normally just say coreFrame in Qt parts where we want to differentiate

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:252
> +    // notImplemented();

I dont get why that is outcommented... either remove the comment or remove the line
Comment 10 WebKit Commit Bot 2011-04-06 17:20:49 PDT
Comment on attachment 88467 [details]
Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp

Clearing flags on attachment: 88467

Committed r83125: <http://trac.webkit.org/changeset/83125>
Comment 11 WebKit Commit Bot 2011-04-06 17:21:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2011-04-06 19:04:05 PDT
http://trac.webkit.org/changeset/83125 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
fast/dom/52776.html
fast/text/complex-text-opacity.html
fast/text/international/bidi-AN-after-L.html
fast/text/international/bidi-AN-after-empty-run.html
fast/text/international/bidi-CS-after-AN.html
fast/text/international/bidi-mirror-he-ar.html
fast/text/international/bidi-neutral-run.html
platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html