Bug 80611 - Move WebNSURLExtras code down to WebCore
Summary: Move WebNSURLExtras code down to WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-08 10:49 PST by Enrica Casucci
Modified: 2012-03-12 09:37 PDT (History)
4 users (show)

See Also:


Attachments
Patch (46.44 KB, patch)
2012-03-08 10:56 PST, Enrica Casucci
ap: review-
Details | Formatted Diff | Diff
Patch2 (92.39 KB, patch)
2012-03-08 15:59 PST, Enrica Casucci
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2012-03-08 10:49:03 PST
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.
Comment 1 Enrica Casucci 2012-03-08 10:56:47 PST
Created attachment 130852 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-08 11:01:52 PST
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.
Comment 3 Enrica Casucci 2012-03-08 11:39:10 PST
Will fix the style before landing the patch.
Comment 4 Alexey Proskuryakov 2012-03-08 11:46:18 PST
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
Comment 5 Enrica Casucci 2012-03-08 11:52:06 PST
(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
Comment 6 Enrica Casucci 2012-03-08 11:53:25 PST
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 7 Alexey Proskuryakov 2012-03-08 12:02:03 PST
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 8 Alexey Proskuryakov 2012-03-08 12:26:11 PST
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.
Comment 9 Enrica Casucci 2012-03-08 15:59:02 PST
Created attachment 130929 [details]
Patch2

New patch per discussion with Alexey.
Comment 10 WebKit Review Bot 2012-03-08 16:02:14 PST
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.
Comment 11 Enrica Casucci 2012-03-08 16:08:08 PST
Fixing the style issues now.
Comment 12 Alexey Proskuryakov 2012-03-08 18:35:57 PST
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?
Comment 13 Enrica Casucci 2012-03-09 09:53:34 PST
(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 14 Alexey Proskuryakov 2012-03-09 12:21:54 PST
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.
Comment 15 Enrica Casucci 2012-03-09 14:47:09 PST
> > 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.
Comment 16 Enrica Casucci 2012-03-09 18:54:29 PST
http://trac.webkit.org/changeset/110360
Comment 17 Csaba Osztrogonác 2012-03-10 01:52:58 PST
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
Comment 18 Csaba Osztrogonác 2012-03-12 09:37:52 PDT
Fixed by http://trac.webkit.org/changeset/110431