Bug 121818

Created attachment 212487 [details] Patch

Attachment 212487 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/ios/EditorIOS.mm', u'Source/WebCore/platform/Pasteboard.h', u'Source/WebCore/platform/PasteboardStrategy.h', u'Source/WebCore/platform/PlatformPasteboard.h', u'Source/WebCore/platform/ios/PasteboardIOS.mm', u'Source/WebCore/platform/ios/PlatformPasteboardIOS.mm', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h', u'Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Scripts/webkit2/messages.py', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/Shared/WebCoreArgumentCoders.h', u'Source/WebKit2/UIProcess/WebContext.h', u'Source/WebKit2/UIProcess/WebContext.messages.in', u'Source/WebKit2/UIProcess/mac/WebContextMac.mm', u'Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h']" exit_code: 1 Source/WebCore/platform/PlatformPasteboard.h:74: The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/PlatformPasteboard.h:75: The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h:86: The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h:87: The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:99: The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:100: The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/WebContext.h:324: The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/WebContext.h:325: The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/PasteboardStrategy.h:47: The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/PasteboardStrategy.h:48: The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 10 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 212487 [details] Patch Attachment 212487 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/2110289

Comment on attachment 212487 [details] Patch Don’t land this if it breaks the build for other platforms ;)

Comment on attachment 212487 [details] Patch Attachment 212487 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1976312

(In reply to comment #4) > (From update of attachment 212487 [details]) > Don’t land this if it breaks the build for other platforms ;) Thanks for the review. I will fix the other platforms before landing.

The failures are unrelated to this patch. Committed revision 156350.

Arrgh, I had a lot of comments on the patch that went along with my review+ and they seemed to all be deleted!

(In reply to comment #7) > The failures are unrelated to this patch. > Committed revision 156350. No the failures weren't unrelated ... Qt EWS noticed the real failure. It seems the generator can't handle the nested ifdefs and generated buggy output. Fix landed in http://trac.webkit.org/changeset/156366

And I broke the Mac build, because I reversed the ifdefs accidentally. Sorry. And AP fixed it - http://trac.webkit.org/changeset/156368. Thanks. The problem was that the generator doesn't like nested ifdefs for some reason. I'm going to file a new bug report about it and attach the generated cpps.

Comment on attachment 212487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212487&action=review > Source/WebCore/platform/Pasteboard.h:30 > +#include "Image.h" Please don’t add this include to Pasteboard.h. It’s not needed. Maybe we need some additional forward class declarations instead? > Source/WebCore/platform/Pasteboard.h:32 > +#include "SharedBuffer.h" Please don’t add this include to Pasteboard.h. It’s not needed. Maybe we need some additional forward class declarations instead? > Source/WebCore/platform/Pasteboard.h:53 > +#if PLATFORM(IOS) I’ve been trying to keep platforms in alphabetical order. So this would go after GTK and before QT, not after WIN. > Source/WebCore/platform/Pasteboard.h:191 > + static String resourceMIMEType(NSString *mimeType); This should take a const String& instead of an NSString *. > Source/WebCore/platform/PasteboardStrategy.h:31 > +#if PLATFORM(IOS) > +#include "Pasteboard.h" > +#endif Conditional includes like this one should go after the unconditional includes in a separate paragraph. But also, I don’t think we should add this include. Instead we should add some forward struct declarations. > Source/WebCore/platform/PasteboardStrategy.h:46 > + // FIXME: we should move Mac to this. Sentence style with capital letter. >> Source/WebCore/platform/PasteboardStrategy.h:47 >> + virtual void writeToPasteboard(const PasteboardWebContent& content) = 0; > > The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot. Please remove this. >> Source/WebCore/platform/PasteboardStrategy.h:48 >> + virtual void writeToPasteboard(const PasteboardImage& pasteboardImage) = 0; > > The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot. Please remove this. > Source/WebCore/platform/PasteboardStrategy.h:49 > + virtual void writeToPasteboard(const String& text) = 0; Please remove this argument name. > Source/WebCore/platform/PlatformPasteboard.h:29 > +#include "Pasteboard.h" We should not add this include. Instead we should add some forward declarations. > Source/WebCore/platform/PlatformPasteboard.h:52 > +#if PLATFORM(IOS) > + PlatformPasteboard(); > +#endif Conditional things like this should go after the the platform-independent ones. > Source/WebCore/platform/PlatformPasteboard.h:53 > + // FIXME: probably we don't need a constructor that takes a pasteboard name for iOS. Comments should have sentence style, with capital letter. >> Source/WebCore/platform/PlatformPasteboard.h:74 >> + void write(const PasteboardWebContent& content); > > The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot. >> Source/WebCore/platform/PlatformPasteboard.h:75 >> + void write(const PasteboardImage& pasteboardImage); > > The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot. > Source/WebCore/platform/PlatformPasteboard.h:76 > + void write(const String& text); I suggest removing the name text here. > Source/WebCore/platform/ios/PasteboardIOS.mm:63 > +// FIXME: the following soft linking and #define needs to be shared > +// with PlatformPasteboardIOS.mm Sentence style with capital letter and period. I also suggest using a single line. > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:2 > + * Copyright (C) 2006-2013 Apple Computer, Inc. All rights reserved. Year ranges are not used. This should list years of publication. And it’s Apple Inc., not Apple Computer, Inc. > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:31 > +#import "config.h" > +#import "Color.h" > +#import "KURL.h" > +#import "Image.h" > +#import "PlatformPasteboard.h" > +#import "SoftLinking.h" The PlatformPasteboard.h should go first, grouped with config.h. The others should be in a separate paragraph. > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:42 > +// FIXME: the following soft linking and #define needs to be shared > +// with PasteboardIOS.mm. Sentence style with capital letter. I also suggest using a single line. > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:159 > + [m_pasteboard setItems:[NSArray arrayWithObject:representations.get()]]; Can’t we use the @[] syntax instead of [NSArray arrayWithObject:]? > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:169 > + [m_pasteboard setItems:[NSArray arrayWithObject:representations.get()]]; Can’t we use the @[] syntax instead of [NSArray arrayWithObject:]? > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:176 > + [m_pasteboard setItems:[NSArray arrayWithObject:representations.get()]]; Can’t we use the @[] syntax instead of [NSArray arrayWithObject:]? > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:793 > +#if PLATFORM(IOS) > +static void encodeSharedBuffer(ArgumentEncoder& encoder, SharedBuffer* buffer) Please leave a blank line to match the blank line before the #endif below. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:817 > + buffer = SharedBuffer::create(static_cast<unsigned char *>(sharedMemoryBuffer->data()), bufferSize); For unsigned char* there should not a be a space after “char”. This looks dangerous. It’s using a buffer size that was read off the wire with the a data pointer from the SharedMemory object. Is this correct? I don’t see how we’re guaranteed that the SharedMemory has the specified size. > Source/WebKit2/UIProcess/WebContext.h:49 > +#include <WebCore/Pasteboard.h> I don’t think this include is needed. We just need forward declarations of a couple classes. >> Source/WebKit2/UIProcess/WebContext.h:324 >> + void writeWebContentToPasteboard(const WebCore::PasteboardWebContent& content); > > The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot on this. >> Source/WebKit2/UIProcess/WebContext.h:325 >> + void writeImageToPasteboard(const WebCore::PasteboardImage& pasteboardImage); > > The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot on this. > Source/WebKit2/UIProcess/WebContext.h:326 > + void writeStringToPasteboard(const String& text); Should remove the word “text”. > Source/WebKit2/UIProcess/mac/WebContextMac.mm:459 > +void WebContext::writeWebContentToPasteboard(const WebCore::PasteboardWebContent &content) The & is in the wrong place. Should be by the type name. > Source/WebKit2/UIProcess/mac/WebContextMac.mm:464 > +void WebContext::writeImageToPasteboard(const WebCore::PasteboardImage &pasteboardImage) I would just use the name image. No need to use the name pasteboardImage. Also, the & is in the wrong place. > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:511 > +void WebPlatformStrategies::writeToPasteboard(const WebCore::PasteboardImage& pasteboardImage) I would just use the name image. No need to use the name pasteboardImage. >> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:99 >> + virtual void writeToPasteboard(const WebCore::PasteboardWebContent& content) OVERRIDE; > > The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot on this. >> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:100 >> + virtual void writeToPasteboard(const WebCore::PasteboardImage& pasteboardImage) OVERRIDE; > > The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot on this. > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:101 > + virtual void writeToPasteboard(const String& text) OVERRIDE; Should remove the argument name text. > Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h:34 > +#if PLATFORM(IOS) > +#include <WebCore/Pasteboard.h> > +#endif Conditional includes should be in a separate paragraph below the unconditional ones. But also, I don’t think this include is needed. Instead we should forward-declare the classes. Further, I don’t think an include like this one needs to be conditional. >> Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h:86 >> + virtual void writeToPasteboard(const WebCore::PasteboardWebContent& content) OVERRIDE; > > The parameter name "content" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot. >> Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h:87 >> + virtual void writeToPasteboard(const WebCore::PasteboardImage& pasteboardImage) OVERRIDE; > > The parameter name "pasteboardImage" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the bot. > Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h:88 > + virtual void writeToPasteboard(const String& text) OVERRIDE; I suggest omitting the argument name “text”. > Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.mm:234 > +void WebPlatformStrategies::writeToPasteboard(const WebCore::PasteboardImage& pasteboardImage) I would just use the name image. No need to use the name pasteboardImage.

Enrica, I rewrote the lost comments that were supposed to go with the review+.

(In reply to comment #12) > Enrica, I rewrote the lost comments that were supposed to go with the review+. Thanks for all the feedback. I will address all your comments in the next upstream patch.

Using the same bug for the next patch.

Created attachment 212840 [details] Second patch

Attachment 212840 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/ios/EditorIOS.mm', u'Source/WebCore/platform/Pasteboard.h', u'Source/WebCore/platform/PasteboardStrategy.h', u'Source/WebCore/platform/PlatformPasteboard.h', u'Source/WebCore/platform/ios/PasteboardIOS.mm', u'Source/WebCore/platform/ios/PlatformPasteboardIOS.mm', u'Source/WebCore/platform/mac/PasteboardMac.mm', u'Source/WebCore/platform/mac/PlatformPasteboardMac.mm', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h', u'Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/Shared/WebCoreArgumentCoders.h', u'Source/WebKit2/UIProcess/WebContext.h', u'Source/WebKit2/UIProcess/WebContext.messages.in', u'Source/WebKit2/UIProcess/mac/WebContextMac.mm', u'Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h']" exit_code: 1 Source/WebCore/platform/PasteboardStrategy.h:46: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/PasteboardStrategy.h:46: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/PasteboardStrategy.h:47: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/PasteboardStrategy.h:47: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/PasteboardStrategy.h:48: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/PasteboardStrategy.h:48: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/PasteboardStrategy.h:49: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/PasteboardStrategy.h:49: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/PasteboardStrategy.h:50: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/PasteboardStrategy.h:50: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/PasteboardStrategy.h:51: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/PasteboardStrategy.h:51: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/PasteboardStrategy.h:52: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/PasteboardStrategy.h:52: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/PasteboardStrategy.h:53: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/PasteboardStrategy.h:53: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 16 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 212843 [details] Second part with fixes I've changed KURL to URL in iOS specific files.

Comment on attachment 212843 [details] Second part with fixes View in context: https://bugs.webkit.org/attachment.cgi?id=212843&action=review > Source/WebCore/editing/ios/EditorIOS.mm:387 > + : frame(frame) > + , context(context) > + , allowPlainText(allowPlainText) > + , madeFragmentFromPlainText(false) Indent. > Source/WebCore/editing/ios/EditorIOS.mm:477 > + NSURL *URL = [NSURL URLWithString:[NSString stringWithFormat:@"%@://%@/%@", @"webkit-fake-url", UUIDString.get(), relativePart]]; > + > + return URL; This could be return [NSURL ...] > Source/WebCore/editing/ios/EditorIOS.mm:494 > + if (url.string().isEmpty()) > + return false; This should be url.isEmpty(). URL::string() is evil :) > Source/WebCore/editing/ios/EditorIOS.mm:607 > + > + Two blank lines here. > Source/WebCore/platform/PasteboardStrategy.h:53 > + No need for the blank line. > Source/WebCore/platform/PasteboardStrategy.h:54 > #endif I would add // PLATFORM(IOS) because this block is a little long. > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:179 > + if (pasteboardType == String(kUTTypeURL)) Unrelated, but we should totally add WTF::String::operator==(NSString*)! > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:180 > + [representations setValue:[[[NSURL alloc] initWithString:text] autorelease] forKey:pasteboardType]; Please use a adoptNS().get() here instead of autorelease. > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:193 > + NSIndexSet *indexSet = [NSIndexSet indexSetWithIndex:index]; RetainPtr<> of [NSIndexSet alloc] initWith...]? > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:204 > + NSIndexSet *indexSet = [NSIndexSet indexSetWithIndex:index]; RetainPtr<> of [NSIndexSet alloc] initWith...]? > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:215 > + if (![value isKindOfClass:[NSString class]]) { > + return String(); > + } Brackets. > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:221 > + NSIndexSet *indexSet = [NSIndexSet indexSetWithIndex:index]; RetainPtr? > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:225 > + if (![pasteboardItem.get() count]) No need for the get(). > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:228 > + id value = [pasteboardItem.get() objectAtIndex:0]; No need for the get(). > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:230 > + ASSERT([value isKindOfClass:[NSURL class]]); This Assert will always hit.

Addressed feedback and landed. Committed revision 156588.