Bug 63155 – Add NSError wrapper functions in ResourceError when USE(CFNETWORK) is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Pratik Solanki]

This is part of the move to the CFNetwork based loader on Mac - bug 51836. In order to send back NSError objects to WebKit on Mac, we need to have a wrapper NSError methods in ResourceError.

[Pratik Solanki]

Created attachment 98202 [details]
Patch

[Darin Adler]

Comment on attachment 98202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98202&action=review

> Source/WebCore/platform/network/mac/ResourceErrorMac.mm:56
> +    RetainPtr<NSDictionary> userInfo(AdoptNS, (NSDictionary *) CFErrorCopyUserInfo(error));

This needs to be AdoptCF, not AdoptNS.

The difference between the two is in garbage collection mode. What matters is the type of retain, CFRetain vs [retain].

> Source/WebCore/platform/network/mac/ResourceErrorMac.mm:57
> +    return [NSError errorWithDomain:(NSString*)CFErrorGetDomain(error) code:CFErrorGetCode(error) userInfo:userInfo.get()];

Don’t we want to cache a single NSError rather than creating new autoreleased one every time? I’m worried about the performance implications of creating new ones every time.

> Source/WebKit/mac/ChangeLog:8
> +        Use the explicit ResourceError::nsError() method instead of implicit operator casts.

Why?

[Pratik Solanki]

(In reply to comment #2)
> (From update of attachment 98202 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98202&action=review
> 
> > Source/WebCore/platform/network/mac/ResourceErrorMac.mm:56
> > +    RetainPtr<NSDictionary> userInfo(AdoptNS, (NSDictionary *) CFErrorCopyUserInfo(error));
> 
> This needs to be AdoptCF, not AdoptNS.

ok

> > Source/WebCore/platform/network/mac/ResourceErrorMac.mm:57
> > +    return [NSError errorWithDomain:(NSString*)CFErrorGetDomain(error) code:CFErrorGetCode(error) userInfo:userInfo.get()];
> 
> Don’t we want to cache a single NSError rather than creating new autoreleased one every time? I’m worried about the performance implications of creating new ones every time.

Yeah, I think we should cache it. Rewriting this part.

> > Source/WebKit/mac/ChangeLog:8
> > +        Use the explicit ResourceError::nsError() method instead of implicit operator casts.
> 
> Why?

I should have explained more in the changelog. I am pretty sure I was getting compile errors because the compiler was confused while trying to convert a ResourceError to an NSError* when USE(CFNETWORK) was enabled. But I am not seeing those anymore. I'll remove those changes.

[Pratik Solanki]

Created attachment 98239 [details]
Patch

[Darin Adler]

Comment on attachment 98239 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98239&action=review

> Source/WebCore/platform/network/mac/ResourceErrorMac.mm:58
> +        RetainPtr<NSDictionary> userInfo(AdoptCF, (NSDictionary *) CFErrorCopyUserInfo(error));
> +        m_platformNSError.adoptNS([[NSError alloc] initWithDomain:(NSString*)CFErrorGetDomain(error) code:CFErrorGetCode(error) userInfo:userInfo.get()]);

Different formatting of the typecasts on two successive lines.

[Pratik Solanki]

Committed r89534: <http://trac.webkit.org/changeset/89534>