Bug 80611 – Move WebNSURLExtras code down to WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Enrica Casucci]

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.

[Enrica Casucci]

Created attachment 130852 [details]
Patch

[WebKit Review Bot]

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.

[Enrica Casucci]

Will fix the style before landing the patch.

[Alexey Proskuryakov]

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

[Enrica Casucci]

(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

[Enrica Casucci]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Enrica Casucci]

Created attachment 130929 [details]
Patch2

New patch per discussion with Alexey.

[WebKit Review Bot]

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.

[Enrica Casucci]

Fixing the style issues now.

[Alexey Proskuryakov]

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?

[Enrica Casucci]

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

[Alexey Proskuryakov]

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.

[Enrica Casucci]

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

[Enrica Casucci]

http://trac.webkit.org/changeset/110360

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Fixed by http://trac.webkit.org/changeset/110431