Bug 40254 – [Qt] FrameLoaderClientQt.cpp has coding-style errors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Bakken]

[Qt] FrameLoaderClientQt.cpp has coding-style errors

[Anders Bakken]

Created attachment 58068 [details]
Patch

[David Levin]

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.

[Jędrzej Nowacki]

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 '.'

[Rafael Brandao]

Created attachment 88445 [details]
Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp

[Alexis Menard (darktears)]

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 `?

[Rafael Brandao]

Created attachment 88460 [details]
My previous patch is now deprecated, some mistaken has been done. Check this one instead.

[WebKit Review Bot]

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.

[Rafael Brandao]

Created attachment 88467 [details]
Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp

[Kenneth Rohde Christiansen]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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