[webkit-reviews] review granted: [Bug 80611] Move WebNSURLExtras code down to WebCore : [Attachment 130929] Patch2

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 80611: Move WebNSURLExtras code down to WebCore
https://bugs.webkit.org/show_bug.cgi?id=80611

Attachment 130929: Patch2
https://bugs.webkit.org/attachment.cgi?id=130929&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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>
> + at class NSString;
> + at 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.