Bug 63155 - Add NSError wrapper functions in ResourceError when USE(CFNETWORK) is enabled
Summary: Add NSError wrapper functions in ResourceError when USE(CFNETWORK) is enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords:
Depends on:
Blocks: 51836
  Show dependency treegraph
 
Reported: 2011-06-22 10:55 PDT by Pratik Solanki
Modified: 2011-06-22 23:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (13.02 KB, patch)
2011-06-22 11:11 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (4.22 KB, patch)
2011-06-22 14:11 PDT, Pratik Solanki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2011-06-22 10:55:08 PDT
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.
Comment 1 Pratik Solanki 2011-06-22 11:11:08 PDT
Created attachment 98202 [details]
Patch
Comment 2 Darin Adler 2011-06-22 11:28:17 PDT
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?
Comment 3 Pratik Solanki 2011-06-22 13:10:34 PDT
(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.
Comment 4 Pratik Solanki 2011-06-22 14:11:04 PDT
Created attachment 98239 [details]
Patch
Comment 5 Darin Adler 2011-06-22 17:30:19 PDT
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.
Comment 6 Pratik Solanki 2011-06-22 23:01:05 PDT
Committed r89534: <http://trac.webkit.org/changeset/89534>