[Qt] FrameLoaderClientQt.cpp has coding-style errors
Created attachment 58068 [details] Patch
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.
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 '.'
Created attachment 88445 [details] Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp
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 `?
Created attachment 88460 [details] My previous patch is now deprecated, some mistaken has been done. Check this one instead.
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.
Created attachment 88467 [details] Fixing previous patch submitted by Anders Bakken to fix the coding-style on FrameLoaderClientQt.cpp
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 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>
All reviewed patches have been landed. Closing bug.
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