This is a cleanup task aimed at improving the overall pasteboard code on Mac. This will allow us to remove the Frame parameter from the Pasteboard class interface in most cases.
Created attachment 130852 [details] Patch
Attachment 130852 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/mac/WebCoreNSStringExtras.h:44: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Will fix the style before landing the patch.
Comment on attachment 130852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130852&action=review This adds code to WebCore, but doesn't switch WebKit to using it. Do you want to do that in a separate patch? I think that this would be easier to review as one patch. > Source/WebCore/platform/mac/WebCoreNSURLExtras.h:34 > +#include <Foundation/Foundation.h> > +@class NSString; > +@class NSURL; You probably meant to remove the #include. > Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:791 > + // journaled the data we're're trying to set. Depending on what other I/O is going on, it can take some Typo: we're're
(In reply to comment #4) > This adds code to WebCore, but doesn't switch WebKit to using it. Do you want to do that in a separate patch? I think that this would be easier to review as one patch. I thought this was easier, since it is a lot of code. Also, I don't plan to change WebKit to use this, only WebKit2. > > You probably meant to remove the #include. Sure. > > Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:791 > > + // journaled the data we're're trying to set. Depending on what other I/O is going on, it can take some > > Typo: we're're
I plan on start using this code for https://bugs.webkit.org/show_bug.cgi?id=80073, then I'll post another patch to remove the use of Frame from the Pasteboard class
Comment on attachment 130852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130852&action=review > Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:157 > + NSBundle *bundle = [NSBundle bundleWithIdentifier:@"com.apple.WebKit"]; I think that this needs stronger error checking now, since we're looking into a different framework. Maybe even as a strong as CRASH.
Comment on attachment 130852 [details] Patch Talked to Enrica in person. We really don't want two copies of this security sensitive code in the project.
Created attachment 130929 [details] Patch2 New patch per discussion with Alexey.
Attachment 130929 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/mac/WebCoreNSStringExtras.h:44: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:44: The parameter name "URL" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:45: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:45: The parameter name "URL" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:46: The parameter name "URL" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:47: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:47: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:48: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:48: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:49: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:49: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:50: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:50: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:51: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:52: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:53: The parameter name "URL" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:53: The parameter name "component" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:54: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:55: The parameter name "URL" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:56: The parameter name "URL" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:56: The parameter name "componentType" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/WebCoreNSURLExtras.h:57: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/mac/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Total errors found: 23 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Fixing the style issues now.
Comment on attachment 130929 [details] Patch2 Yup, this looks like what I had in mind overall. What aspect of this did you want to discuss?
(In reply to comment #12) > (From update of attachment 130929 [details]) > Yup, this looks like what I had in mind overall. What aspect of this did you want to discuss? Nothing, I mentioned that I was uploading a patch as result of our conversation.
Comment on attachment 130929 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=130929&action=review I don't know how I managed to read "per discussion" as "for discussion" :) r=me with some comments. > Source/WebCore/WebCore.exp.in:1516 > +_originalData I don't think that this is a good name to export in global namespace. These are not even used in any .m files AFAICT, so you can just remove 'extern "C"' from the new WebCore function prototypes. > Source/WebCore/platform/mac/FileSystemMac.mm:78 > +static void *setMetaData(void* context) Misplaced star in returned type. > Source/WebCore/platform/mac/FileSystemMac.mm:81 > + NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init]; I'm not sure if it's actually needed for anything. Can you remove the pool, and see if Objective C runtime complains? > Source/WebCore/platform/mac/FileSystemMac.mm:82 > + wkSetMetadataURL((NSString *)info->URLString, (NSString *)info->referrer, (NSString *)info->path); The path should be passed through fileSystemRepresentation before being used outside WebKit (for consistency if nothing else). > Source/WebCore/platform/mac/FileSystemMac.mm:111 > + MetaDataInfo *info = static_cast<MetaDataInfo *>(malloc(sizeof(MetaDataInfo))); It seems that you can use new (and RefPtr members) for this structure. We have full C++ powers! > Source/WebCore/platform/mac/WebCoreNSURLExtras.h:34 > +#include <Foundation/Foundation.h> > +@class NSString; > +@class NSURL; This looks wrong. Why both include and forward declare? > Source/WebCore/platform/mac/WebCoreNSURLExtras.h:37 > +typedef struct NSString NSString; > +typedef struct NSURL NSURL; We have OBJ_CLASS macro for this now. > Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:159 > + if (!bundle) > + CRASH(); It might be better to have the check when file doesn't exist, in case we forget to master it into WebKit. We didn't really have such check before (discarding the error silently), but why not add it now? > Source/WebKit/mac/Misc/WebNSURLExtras.mm:49 > -// Needs to be big enough to hold an IDN-encoded name. > -// For host names bigger than this, we won't do IDN encoding, which is almost certainly OK. > -#define HOST_NAME_BUFFER_LENGTH 2048 > - > #define URL_BYTES_BUFFER_LENGTH 2048 What is URL_BYTES_BUFFER_LENGTH still used for? Does it need a comment? > Source/WebKit/mac/WebCoreSupport/WebSystemInterface.mm:172 > + INIT(SetMetadataURL); Eventually, you'll need the same for WK2.
> > Source/WebCore/WebCore.exp.in:1516 > > +_originalData > > I don't think that this is a good name to export in global namespace. These are not even used in any .m files AFAICT, so you can just remove 'extern "C"' from the new WebCore function prototypes. It is used in WebKit WebNSURLExtras.mm in several places. I'll change the name to orginalURLData. > > Source/WebCore/platform/mac/FileSystemMac.mm:78 > > +static void *setMetaData(void* context) > > Misplaced star in returned type. Done. > > Source/WebCore/platform/mac/FileSystemMac.mm:81 > > + NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init]; > > I'm not sure if it's actually needed for anything. Can you remove the pool, and see if Objective C runtime complains? It doesn't and I've removed it. > > Source/WebCore/platform/mac/FileSystemMac.mm:82 > > + wkSetMetadataURL((NSString *)info->URLString, (NSString *)info->referrer, (NSString *)info->path); > > The path should be passed through fileSystemRepresentation before being used outside WebKit (for consistency if nothing else). > > > Source/WebCore/platform/mac/FileSystemMac.mm:111 > > + MetaDataInfo *info = static_cast<MetaDataInfo *>(malloc(sizeof(MetaDataInfo))); > > It seems that you can use new (and RefPtr members) for this structure. We have full C++ powers! Ok. > > Source/WebCore/platform/mac/WebCoreNSURLExtras.h:34 > > +#include <Foundation/Foundation.h> Removed. > > Source/WebCore/platform/mac/WebCoreNSURLExtras.h:37 > > +typedef struct NSString NSString; > > +typedef struct NSURL NSURL; > > We have OBJ_CLASS macro for this now. Done. > > > Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:159 > > + if (!bundle) > > + CRASH(); > > It might be better to have the check when file doesn't exist, in case we forget to master it into WebKit. We didn't really have such check before (discarding the error silently), but why not add it now? Will do. > > > What is URL_BYTES_BUFFER_LENGTH still used for? Does it need a comment? > It is used by _web_URLWithLowercasedScheme. > > Source/WebKit/mac/WebCoreSupport/WebSystemInterface.mm:172 > > + INIT(SetMetadataURL); > > Eventually, you'll need the same for WK2. I had already done it, it wasn't added to the patch by mistake. I'll add it when landing.
http://trac.webkit.org/changeset/110360
Reopen, because it broke Qt Mac build: In file included from ../../../../Source/WebCore/platform/mac/WebCoreObjCExtras.mm:35: ../../../../Source/WebCore/platform/mac/WebCoreObjCExtras.h:39: error: ‘WebCoreCFAutorelease’ declared as an ‘inline’ variable ../../../../Source/WebCore/platform/mac/WebCoreObjCExtras.h:39: error: ‘CFTypeRef’ was not declared in this scope ../../../../Source/WebCore/platform/mac/WebCoreObjCExtras.h:40: error: expected ‘,’ or ‘;’ before ‘{’ token
Fixed by http://trac.webkit.org/changeset/110431