Bug 40239 – [Qt] ChromeClientQt.h has coding-style errors

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

[Anders Bakken]

[Qt] ChromeClientQt.h has coding-style errors

[Anders Bakken]

Created attachment 58050 [details]
Patch

[David Levin]

Comment on attachment 58050 [details]
Patch

WebKit/qt/WebCoreSupport/ChromeClientQt.h:36
 +  #include "RefCounted.h"
#include <wtf/RefCounted.h>

[Nancy Piedra]

Created attachment 87656 [details]
ChromeClientQt.h style fixes

It's been a long time since the original patch was submitted so I created a new patch.

[WebKit Review Bot]

Attachment 87656 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nancy Piedra]

(In reply to comment #4)
> Attachment 87656 [details] did not pass style-queue:
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1
> Source/WebKit/qt/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> Total errors found: 1 in 2 files
> If any of these errors are false positives, please file a bug against check-webkit-style.

There is a bug number in the Changelog so I'm not sure what is missing here.

[Benjamin Poulain]

Comment on attachment 87656 [details]
ChromeClientQt.h style fixes

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

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.h:37
> +#include "RefCounted.h"

That should be #include <wtf/RefCounted.h>

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.h:100
> +    virtual void addMessageToConsole(MessageSource, MessageType, MessageLevel, const String& message,
> +                                     unsigned int lineNumber, const String& sourceID);

You can put this on a single line, we don't limit line to 80 characters in WebKit.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.h:152
> +    // see ChromeClient.h

You can remove that line.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.h:153
> +    // this is a hook for WebCore to tell us what we need to do with the GraphicsLayers

Comments should be sentences, with a upper case at start, and a dot at the end.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.h:200
> +    WebCore::KURL lastHoverURL;
> +    WTF::String lastHoverTitle;
> +    WTF::String lastHoverContent;

Where are those prefixed by the namespace? Aren't we in the WebCore namespace already? And from the API is looks like WTF is also defined.

[Nancy Piedra]

Created attachment 87717 [details]
New patch to fix ChromeClientQt.h style issues

I have uploaded a new patch based on the comments.  I still get style error that the Changelog does not have a bug number but it does so I'm not sure what to do about that.

[WebKit Review Bot]

Attachment 87717 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 87717 [details]
New patch to fix ChromeClientQt.h style issues

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

Looks good. Just check the namespace question before pushing.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.h:113
> +    virtual WebCore::KeyboardUIMode keyboardUIMode();

Is the WebCore namespace needed here?

[Benjamin Poulain]

(In reply to comment #7)
> I have uploaded a new patch based on the comments.  I still get style error that the Changelog does not have a bug number but it does so I'm not sure what to do about that.

Check if there a character that should not be here in the checklog. It could be a \t or an non-breaking space in there.

[Nancy Piedra]

Created attachment 87724 [details]
New patch to fix Changelog style issue and namespace issue.

I uploaded a new patch that fixes the Changelog style issue and namespace issue.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 87724 [details]:

animations/suspend-resume-animation.html bug 48161 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 87724 [details]
New patch to fix Changelog style issue and namespace issue.

Clearing flags on attachment: 87724

Committed r82585: <http://trac.webkit.org/changeset/82585>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.